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

make snapshot lazier and fix defaultValue #5428

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Apr 10, 2018

Resolves #5426
Resolves #5419

Adds tests for

    1. test that a record is not materialized (e.g. internalModel.hasRecord is false) when calling findRecord for the very first time
    1. test that defaultValues provided to a model reflect on attributes on snapshots (both function based and value based)
    1. test that non-materialized records generate attributes for snapshots lazily
    1. test that materialized records generate attributes for snapshots greedily
    1. unskips the test for snapshots not greedily causing model classes to be looked up

@runspired runspired changed the title [WIP] [WIP] make snapshot lazier and fix defaultValue Apr 10, 2018
@runspired
Copy link
Contributor Author

cc @pangratz @rwjblue

@runspired runspired changed the title [WIP] make snapshot lazier and fix defaultValue make snapshot lazier and fix defaultValue Apr 10, 2018
adds an explicit test for laziness during findRecord

cleanup test
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@@ -17,18 +17,23 @@ import { get } from '@ember/object';
*/
export default class Snapshot {
constructor(internalModel, options = {}) {
this._attributes = Object.create(null);
this.__attributes = null;
Copy link
Member

Choose a reason for hiding this comment

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

I'd like (at some point) to start discussing moving some of these state caches into weakmaps now that we can use them indiscriminately...

Copy link
Member

Choose a reason for hiding this comment

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

I can try and tackle that enhancement... maybe use a similar weakmap implementation found elsewhere in the code?

@runspired runspired merged commit c438a8a into master Apr 10, 2018
@runspired runspired deleted the fix-snapshot-default-value branch April 10, 2018 22:59
@urbany
Copy link

urbany commented Apr 26, 2018

Hi, thanks for this fix! Is it possible to include it in the next beta?

sungraze added a commit to occrp/id-frontend that referenced this pull request May 17, 2018
Failing test coming from emberjs/data#5444 so we need at least ember-data@3.2.0-beta-2.

Using 3.2.0.beta-2 we get a fail related to defaultValue emberjs/data#5428 so we need a new beta :)

Also maybe-import-regenerator seems to not import the generator (?) so I’ve added `includePolyfill: true` on babel opts.

(mirage) Serialized rels on ticket reload to match the back-end.

Otherwise fine, RIP my hype.
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.

None yet

4 participants