[BUGFIX] Fix availability of properties in createRecord init #5413

runspired · 2018-04-03T23:42:01Z

This restores the changes from #4931 that were removed by #5369, ensuring that in as many cases as possible we "do the expected thing" and give access to properties in init.

Historically, so long as _internalModel has data, any available record-data is available in init. This PR ensures that the same is true for properties passed to createRecord.

We achieve this by buffering the changes for dirty-state sent to ManyArray. In the process, we also cleanup the responsibilities of attr belongsTo and hasMany descriptors who previously were responsible for the tricky logic of where to find state and how to set state on internal-model. This should help inform and improve the APIs of emberjs/rfcs#293

runspired · 2018-04-03T23:42:37Z

cc @dwickern if you could "sanity check" this change in your application that hit issue before. I've kept your test and added additional coverage.

runspired · 2018-04-03T23:44:36Z

This should resolve #5247 as well, although not by making the API async but by ensuring that createRecord joins ember-data's internal backburner loop to setup relationships.

Edit We still need to wrap it in an Ember.run as well. We previously agreed to do so, I'll follow this PR up with one that does and cleans up test code that wraps createRecord in run loops.

dwickern · 2018-04-04T00:16:02Z

I'm still on 2.x so it might be difficult to test this

rwjblue

Can you ensure tests exist that ensure the more common ways to instantiate a record (e.g. store.findRecord('post', 1) et al) also passes properties to record init?

rwjblue · 2018-04-04T13:30:09Z

addon/-private/system/model/internal-model.js

@@ -348,6 +350,38 @@ export default class InternalModel {
        isError: this.isError,
        adapterError: this.error
      };
+      let hasCreateProperties = typeof properties === 'object' && properties !== null;


I would think properties !== undefined would be fine here...

It might be, depends on how defensive we want to be against things like createRecord('foo', null) or similar. We could check undefined and then assert this check though to preserve perf.

rwjblue · 2018-04-04T13:51:36Z

addon/-private/system/model/internal-model.js

+          }
+        });
+
+        emberAssign(createOptions, properties);


I would prefer to avoid mutating the provided POJO (mutating for example, completely breaks using a shared POJO to .createRecord across many tests). A straightforward way to do this would be to invert this method a bit:

if (hasCreateProperties) { let classFields = this.getFields(); for (let propertyName in properties) { let propertyValue = properties[propertyName]; let fieldType = classFields.get(propertyName); if (propertyName === 'id') { this.setId(properties.id); continue; } switch (fieldType) { case 'attribute': this.setDirtyAttribute(name, propertyValue); break; case 'belongsTo': this.setDirtyBelongsTo(name, propertyValue); initializedRelationships.push(name); break; case 'hasMany': this.setDirtyHasMany(name, propertyValue); initializedRelationships.push(name); break; default: createOptions[propertyName] = propertyValue; } } } // ...snip... getFields(attributeName) { return get(this.modelClass, 'fields'); }

I noticed last night that we actually have a test for using a shared pojo with createRecord and for WTF reasons it appears to still pass. I will be upgrading that test.

In regards to the proposed new direction, I like it.

rwjblue · 2018-04-04T13:58:42Z

addon/-private/system/model/internal-model.js

@@ -358,6 +392,12 @@ export default class InternalModel {

      this._record = this.store.modelFactoryFor(this.modelName).create(createOptions);

+      if (hasCreateProperties && initializedRelationships.length) {
+        for (let i = 0; i < initializedRelationships.length; i++) {
+          this._relationships.get(initializedRelationships[i]).setHasData(true);


Hmm, why doesn't setDirtyHasMany / setDirtyBelongsTo mark the relationship as having data? Seems vaguely surprising to me...

We can move this into the original iteration (I believe), but not into the methods. setDirtyHasMany and setDirtyBelongsTo are used for update code-paths as well as create-code-paths and we only mark this as hasData for creates.

rwjblue · 2018-04-04T18:38:10Z

addon/-private/system/relationships/state/has-many.js

+    this._willUpdateManyArray = true;
+    let backburner = this.store._backburner;
+
+    backburner.join(() => {


IMHO, it would be nice to remove this (and rely on the auto-run). What do you think?

The problem is that too many of our APIs are sync, so we can't rely on setTimeout (or microtasks if backburner changes) for settledness at the moment. We need the work to complete by the time someone exits the ember-data world.

This is something we would like to change, and a longterm goal of ours is to move more public APIs to being promise based to unlock optimizations around batching and updating without needing this sort of wonkiness.

…utated

…possible, skip test that should have never worked but also improve it so that we can maybe make it work in the future

rwjblue · 2018-04-04T21:49:29Z

addon/-private/system/snapshot.js

@@ -24,25 +24,13 @@ export default class Snapshot {
    this._hasManyIds = Object.create(null);
    this._internalModel = internalModel;

-    let record = internalModel.getRecord();
+    // TODO is there a way we can assign known attributes without


Can you make an issue so we can track fixing (or removing the TODO)?

i left the skipped test for tracking :D
i can open an issue too though

Issue is #5419

rwjblue · 2018-04-04T21:51:11Z

addon/-private/system/store.js

-    // to avoid conflicts.
+    return this._backburner.join(() => {
+      let normalizedModelName = normalizeModelName(modelName);
+      let properties = copy(inputProperties) || Object.create(null);


Just notating that we will eventially need to deal with the Ember.copy deprecation that is being worked on in emberjs/rfcs#322. Nothing needs to be done now though (AFAICT)...

yeah, we know. we use it a ton :/

pangratz · 2018-04-09T12:04:23Z

addon/-private/system/snapshot.js

+    //  using `eachAttribute`? This forces us to lookup the model-class
+    //  but for findRecord / findAll these are empty and doing so at
+    //  this point in time is unnecessary.
+    internalModel.eachAttribute((keyName) => this._attributes[keyName] = internalModel.getAttributeValue(keyName));


Looks like this broke defaultValue not being considered anymore, when the attribute hasn't been accessed yet 😔

So given:

// app/models/foo.js export default Model.extend({ bar: attr({ defaultValue: () => "baz" }) });

then

let foo = store.createRecord('foo'); // this assertion breaks after upgrading to v3.2.0-beta.2 :pensive: assert.deepEqual(foo.toJSON(), { bar: "baz" });

Currently the whole story around defaultValue is not ideal (e.g. #2566) and doing this in the app code by explicitly passing default values when creating a record is better, but I would still consider this a breaking change... What do you think?

Seems good to open an issue for this, I think it should be fixable...

Should be very fixable, surprised this broke though, re-examining my changes, will open an issue.

hrm, so I didn't pull the defaultValue logic into internalModel, nor am I sure we should. https://github.com/emberjs/data/pull/5413/files#diff-c2a90350f555423721228dfa13c137f2R130

That said, the proposal in #5419 would be a good direction to address this, as we can access each attribute via the record if it exists, and only copy lazily if it does not.

runspired requested review from stefanpenner, rwjblue, hjdivad, igorT and mmun April 3, 2018 23:42

runspired changed the title ~~[BUFXIF] Fix availability of properties in createRecord init~~ [BUGFIX] Fix availability of properties in createRecord init Apr 3, 2018

mmun assigned mmun and unassigned mmun Apr 4, 2018

mmun approved these changes Apr 4, 2018

View reviewed changes

rwjblue reviewed Apr 4, 2018

View reviewed changes

runspired added 11 commits April 4, 2018 14:11

refactor attr to enable internalModel to set dirty state directly

22d481a

refactor hasMany/belongsTo to enable internal-model to setup dirty state

4b32222

adds a failing test for createRecord with properties

64f2db1

lets begin by just making internal state setup work

1cbacd3

go back to initializing through the record

ec2f6df

fix setId behavior on createRecord

0739672

make it all work

74476c8

refactor implementation to not mutate properties

5fbbfab

expand test ensuring that properties passed to createRecord are not m…

684f10b

…utated

add additional test coverage

ba094ee

fix tests for findRecord by making snapshot lazily call getRecord if …

d578710

…possible, skip test that should have never worked but also improve it so that we can maybe make it work in the future

runspired force-pushed the fix-create-record-init branch from 426b98e to d578710 Compare April 4, 2018 21:11

address failing test post-rebase

de55378

rwjblue approved these changes Apr 4, 2018

View reviewed changes

runspired merged commit 7b5a885 into master Apr 4, 2018

runspired deleted the fix-create-record-init branch April 4, 2018 23:14

runspired mentioned this pull request Apr 6, 2018

Incorrect attributeBindings for Ember model #4542

Closed

pangratz reviewed Apr 9, 2018

View reviewed changes

runspired mentioned this pull request Apr 9, 2018

Latest beta does not include defaultValue on Snapshots #5426

Closed

runspired mentioned this pull request Apr 21, 2018

data binding with relationships seems broken >= 3.0.0 #5444

Closed

dwickern mentioned this pull request May 1, 2018

Not working with ember-data v3.2.0-beta.2 adopted-ember-addons/ember-data-model-fragments#296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Fix availability of properties in createRecord init #5413

[BUGFIX] Fix availability of properties in createRecord init #5413

runspired commented Apr 3, 2018

runspired commented Apr 3, 2018

runspired commented Apr 3, 2018 •

edited

Loading

dwickern commented Apr 4, 2018

rwjblue left a comment

rwjblue Apr 4, 2018

runspired Apr 4, 2018

rwjblue Apr 4, 2018

runspired Apr 4, 2018

rwjblue Apr 4, 2018

runspired Apr 4, 2018

rwjblue Apr 4, 2018

runspired Apr 4, 2018

rwjblue Apr 4, 2018

runspired Apr 4, 2018

runspired Apr 9, 2018

rwjblue Apr 4, 2018

runspired Apr 4, 2018

pangratz Apr 9, 2018 •

edited

Loading

rwjblue Apr 9, 2018

runspired Apr 9, 2018

runspired Apr 9, 2018

[BUGFIX] Fix availability of properties in createRecord init #5413

[BUGFIX] Fix availability of properties in createRecord init #5413

Conversation

runspired commented Apr 3, 2018

runspired commented Apr 3, 2018

runspired commented Apr 3, 2018 • edited Loading

dwickern commented Apr 4, 2018

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pangratz Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Apr 3, 2018 •

edited

Loading

pangratz Apr 9, 2018 •

edited

Loading