Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ParseObject): enable subclasses to set initial values for props #924

Open
wants to merge 7 commits into
base: alpha
Choose a base branch
from

Conversation

RaschidJFR
Copy link
Contributor

@RaschidJFR RaschidJFR commented Sep 16, 2019

Sobclasses of ParseObject where unable to set initial values for their props and maintain them after fetching from the server. The function ParseObject.fromJSON() has been modified to allow this feature. Two test cases have been added.

It may close parse-community/parse-server#5521

* Test failing with error "Expected 'default' to be 'foo'."
* `jasmine.DEFAULT_TIMEOUT_INTERVAL` was increased to `10000` because the first cases often failed when starting integration testing.

Related to issue parse-community/parse-server#5521
I found that the problem is in the `ParseObject` class
Sobclasses of `ParseObject` where unable to set initial values for their props and maintain them after fetching from the server. The function `ParseObject.fromJSON()` has been modified to allow this feature. Two test cases have been added.

It may close parse-community/parse-server/#5521
@codecov
Copy link

codecov bot commented Oct 27, 2019

Codecov Report

Merging #924 into master will decrease coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #924      +/-   ##
=========================================
- Coverage   91.87%   91.5%   -0.38%     
=========================================
  Files          53      53              
  Lines        5073    5073              
  Branches     1144    1144              
=========================================
- Hits         4661    4642      -19     
- Misses        412     431      +19
Impacted Files Coverage Δ
src/ParseObject.js 89.31% <100%> (-0.12%) ⬇️
src/ParseUser.js 79.36% <0%> (-2.73%) ⬇️
src/ParseQuery.js 94.83% <0%> (-1.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe5e21a...8c9bf87. Read the comment docs.

@@ -1701,7 +1701,7 @@ class ParseObject {
throw new Error('Cannot create an object without a className');
}
const constructor = classMap[json.className];
const o = constructor ? new constructor() : new ParseObject(json.className);
const o = constructor ? new constructor(json) : new ParseObject(json.className, Object.assign(json, { className: undefined }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const o = constructor ? new constructor(json) : new ParseObject(json.className) should be fine for your test to work.

There are other issues this brings up. There are other Subclasses of ParseObject like _User and _Session that will try to set readonly fields. You could try to clone the json and delete the readonly fields before passing to the constructor but I don't know what other issue this would cause.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RaschidJFR Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry I didn’t reply earlier. I’ve already done that with good results. Haven’t pushed though.

I’ve got almost all tests passing now, just a couple left. What I’m trying to figure out now is a way to prevent pending ops from overwriting the properties set in the constructor.

Tomorrow I’ll push my progress so anyone can have a look.

dplewis and others added 2 commits November 5, 2019 22:42
* reserved field prevented the creation of some classes such as ParseUser
* fix test 'retrieve subclass objects' in ParseObjectTest.js
@RaschidJFR
Copy link
Contributor Author

@dplewis So I pushed my last commit... it turns out that it was just what you suggested. I havn't continued working on it since a couple of weeks so I can't remember exactly where I am, I'll retake it hopefully soon.

For now there are only a couple of tests failing for ParseObject: 'conver from JSON' and `retrieve subclass objects' (seems like we're back to square one). I just need to figure out how to keep the attributes from _finishFetch() over the PendingOps.

@victorx98
Copy link

@RaschidJFR @dplewis Any update on this PR? Are we going to merge it anytime soon? Thanks!

@RaschidJFR
Copy link
Contributor Author

Hi @victorx98 Sorry, work is keeping me busy so I won't be able to re-take it at least for the next 4 weeks. I see there's been increasing activity around this topic lately so I will dive into it when I'm out of homework. Meanwhile anyone how wishes to retake the subject is welcome to give a hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse.Query.find() returns Parse.Pointer[] instead of Parse.Object[]
3 participants