Updating references that use internalModel to use recordData #324

cohitre · 2019-01-12T21:00:37Z

EmberData 3.5 introduced the RecordData object inside the InternalModel as a way to organize the internal state of models. This made this plugin incompatible with the more recent versions of EmberData #315

This PR is a first attempt at updating the plugin to be compatible with EmberData 3.7.

Some of the big changes in EmberData that I've encountered:

recordData now encapsulates _data, _attributes and _inFlightAttributes: I've updated all of the references to these three objects to go through _recordData. I've also moved the _fragments, _owner and _name properties into _recordData for consistency.
The InternalModel#flushChangedAttributes method doesn't exist anymore: It seems to have been replaced with willCommit, which is then delegated to _recordData. I am overriding willCommit directly on the RecordData prototype.

TODO:

Decorate the recordData overrides properly and remove the copy/pasted implementation.
Fix currentState assertion when creating a record with fragments.

cohitre · 2019-01-12T21:06:24Z

The only snag that I am currently hitting is with this error showing up in some of the tests:

Error: 'currentState' is a reserved property name on instances of classes extending Model. Please choose a different property name for model:user

That error is triggered by the following line: https://github.com/emberjs/data/blob/v3.7.0/addon/-private/system/model/model.js#L1226. It seems to be because the currentState descriptor doesn't have the writable property set to on as expected by the isBasicDesc function: https://github.com/emberjs/data/blob/v3.7.0/addon/-private/system/model/model.js#L1182

I have ran this with a debugger statement and then set the writable property to true and I can confirm that all the tests pass under those circumstances. I am stumped as to what could be changing this property since nothing in the plugin seems to override that descriptor.

cohitre · 2019-01-13T18:32:26Z

It seems like this assertion is only triggered when instantiating fragments on record creation. At some point when the fragment is set the currentState on the parent record is changed. The value of the currentState itself seems to be correct, but the property definition is not.

cohitre · 2019-01-14T16:29:43Z

addon/ext.js

-
-  @method rollbackAttributes
-*/
-decorateMethod(InternalModelPrototype, 'rollbackAttributes', function rollbackFragments() {


Deleting this method and moving its functionality into RecordData. InternalModel now delegates to the rollbackAttributes method in the recordData object.

cohitre · 2019-01-14T16:30:21Z

addon/ext.js

-
-  @method flushChangedAttributes
-*/
-decorateMethod(InternalModelPrototype, 'flushChangedAttributes', function flushChangedAttributesFragments() {


The functionality in this method seems to have been moved to willCommit

cohitre · 2019-01-14T16:30:49Z

addon/ext.js

-
-  @method adapterDidCommit
-*/
-decorateMethod(InternalModelPrototype, 'adapterDidCommit', function adapterDidCommitFragments(returnValue, args) {


Moved this functionality to recordData

cohitre · 2019-01-14T17:31:25Z

addon/states.js

@@ -164,24 +164,23 @@ FragmentRootState = wireState(FragmentRootState, null, 'root');
 export default FragmentRootState;

 export function fragmentDidDirty(record, key, fragment) {
-  if (!get(record, 'isDeleted')) {


This can get called while the record is still being initialized, which causes the DS.Model#currentState to be overwritten with a new descriptor which then triggers an assertion in the DS.Model#init method. I am not sure what the consequences of accessing the currentState directly could be. Can we expect it to always be the canonical state?

cohitre · 2019-01-14T19:15:41Z

addon/states.js


  // Don't reset if the record is new, otherwise it will enter the 'deleted' state
  // NOTE: This case almost never happens with attributes because their initial value
  // is always undefined, which is *usually* not what attributes get 'reset' to
-  if (!get(record, 'isNew')) {
+  if (!record.currentState.isNew) {


Any potential issues with accessing currentState directly? Would using internalModel._currentState be a better idea?

It seems like it should be ok

jakesjews · 2019-01-15T14:04:35Z

@workmanw this looks good to me how about you?

cohitre · 2019-01-15T15:13:16Z

Now that all the tests are passing I have been testing this against my application. It seems to have introduced an issue where some shared references don't get setup correctly when loading the data.

The problem could be because of this update or because of the EmberData upgrade. I can reproduce that behavior with 3.5, 3.6 and 3.7. I'm investigating that issue and I'll update once I've figured out the cause.

cohitre · 2019-01-17T14:58:37Z

After much debugging I've figured that the issue I was seeing was in my application and was a result of upgrading to EmberData 3.5. It seems like the behavior of inverse relationships is a bit different in EmberData 3.5 and I had to update some relationships that should've been set to null. This gives me added confidence on this PR.

It's important to note that this PR breaks compatibility with versions of EmberData before 3.5 and that it may be worth refactoring this to reduce the footprint of the core models that are extended.

Can the Model#willDestroy override be replaced with a RecordData#unloadRecord override?
Can the InternalModel#adapterDidError override be replaced with a RecordData#commitWasRejected override?

@jakesjews @workmanw How do you want to proceed?

jakesjews · 2019-01-20T20:45:17Z

I'm ok with getting this merged then if its needed we can tweak the implementation. @workmanw how about you?

workmanw · 2019-01-21T01:40:48Z

@jakesjews Looks good to me. I am not concerned about the backwards compat. That comes with the territory for this addon.

Thanks a ton for diving in @cohitre .

jakesjews · 2019-01-21T03:07:04Z

@workmanw I think we should release this as version 4 so we don't catch anyone by surprise how about you?

BnitoBzh · 2019-01-24T12:28:07Z

Merged, wonderful ! when the new version will be available (with README.md update) ?

workmanw · 2019-01-24T13:43:07Z

Sorry, I'll try to get to this today. I got pulled into a bunch of issues yesterday.

cohitre added 4 commits January 11, 2019 15:29

Updating references that use internalModel to use recordData

65ee33a

Fixing how states are cleaned up after save

e78be56

Overriding the changedAttributes method in recordArray

dd11d65

Moving the rollbackAttributes override back to the internalModel

a9d525a

cohitre commented Jan 14, 2019

View reviewed changes

Directly accessing the currentState to prevent it from being rewritten

e911cb3

cohitre commented Jan 14, 2019

View reviewed changes

Cleaning up overrides so that the super function gets called

d2c9122

cohitre commented Jan 14, 2019

View reviewed changes

cohitre added 2 commits January 14, 2019 15:54

Adding tests for all recent versions of ember

9f40ca5

Linting the config file

f86b967

erkie mentioned this pull request Jan 15, 2019

not working with 3.5 beta #315

Closed

jakesjews merged commit c00e2b1 into adopted-ember-addons:master Jan 21, 2019

XaserAcheron mentioned this pull request Jan 24, 2019

Alpha release w/ember-data 3.5+ support? #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating references that use internalModel to use recordData #324

Updating references that use internalModel to use recordData #324

cohitre commented Jan 12, 2019 •

edited

Loading

cohitre commented Jan 12, 2019

cohitre commented Jan 13, 2019

cohitre Jan 14, 2019

cohitre Jan 14, 2019

cohitre Jan 14, 2019

cohitre Jan 14, 2019

cohitre Jan 14, 2019

jakesjews Jan 14, 2019

jakesjews commented Jan 15, 2019

cohitre commented Jan 15, 2019

cohitre commented Jan 17, 2019

jakesjews commented Jan 20, 2019

workmanw commented Jan 21, 2019

jakesjews commented Jan 21, 2019

BnitoBzh commented Jan 24, 2019 •

edited

Loading

workmanw commented Jan 24, 2019

Updating references that use internalModel to use recordData #324

Updating references that use internalModel to use recordData #324

Conversation

cohitre commented Jan 12, 2019 • edited Loading

cohitre commented Jan 12, 2019

cohitre commented Jan 13, 2019

cohitre Jan 14, 2019

Choose a reason for hiding this comment

cohitre Jan 14, 2019

Choose a reason for hiding this comment

cohitre Jan 14, 2019

Choose a reason for hiding this comment

cohitre Jan 14, 2019

Choose a reason for hiding this comment

cohitre Jan 14, 2019

Choose a reason for hiding this comment

jakesjews Jan 14, 2019

Choose a reason for hiding this comment

jakesjews commented Jan 15, 2019

cohitre commented Jan 15, 2019

cohitre commented Jan 17, 2019

jakesjews commented Jan 20, 2019

workmanw commented Jan 21, 2019

jakesjews commented Jan 21, 2019

BnitoBzh commented Jan 24, 2019 • edited Loading

workmanw commented Jan 24, 2019

cohitre commented Jan 12, 2019 •

edited

Loading

BnitoBzh commented Jan 24, 2019 •

edited

Loading