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

[BUGFIX beta] Fix createRecord creating two records #5369

Merged
merged 3 commits into from
Mar 4, 2018

Conversation

dwickern
Copy link
Contributor

@dwickern dwickern commented Mar 1, 2018

Fixes #5359 and emberjs/ember.js#16258

Explained in #5364:

Before this PR, it was possible to accidentally reify inverse relationships into an invalid state when combining createRecord with @each observers/CPs on an inverse relationship (see test). This would cause internalModel's getRecord to be called recursively with unexpected results. This problem became amplified in Ember 3.x because ArrayProxies now cache the result of objectAt, which lead us to caching the wrong invalid state.

This applies the workaround from #5359 (comment) to create and cache the record before setting any state which might update inverses.

cc @mmun

Copy link
Member

@mmun mmun left a comment

Choose a reason for hiding this comment

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

Thanks @dwickern! Can you also remove the argument and related code from InternalModel#getRecord? It's no longer used.

@mmun
Copy link
Member

mmun commented Mar 2, 2018

FYI, the CI failure is unrelated to this PR.

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.

Thanks for working on this!

To be clear, this reverts the changes in #4931 which in hindsight were not fully correct. There were still many pathways that would create a model without properties so the reported issue in #4509 was still an issue if you ever used that specific model via normal findRecord / findAll / etc mechanisms (which I daresay is massively more likely than the cases where the user manually calls .createRecord).

I believe this is the correct path forward.

@dwickern
Copy link
Contributor Author

dwickern commented Mar 2, 2018

I'll add that 2 records were being created on ember 2.x as well due to #4931. The extra record was discarded (leaked?) until the ArrayProxy change in ember 3.0 made the behavior visible.

Thanks @mmun for all your work on this!

Do you want a separate PR backporting to ember-data 2.18.x?

@rwjblue
Copy link
Member

rwjblue commented Mar 4, 2018

@dwickern - I think the issues with master should be resolved, would you mind a rebase?

@mmun
Copy link
Member

mmun commented Mar 4, 2018

@rwjblue I rebased.

@aaronbhansen
Copy link

I'm getting this on Ember 2.18.3. I applied this patch locally and it fixed my problem. @rwjblue @mmun any issue with me opening a PR for this same fix on 2.18?

See this image.
image

Both order_selection and $E are order/selection models. They have different emberIds, but the same internal model.

My component has a reference to the order_selection from the initial event, but the store has a different model. the order_selection my component is referencing isn't actually found in the store anywhere.

@runspired
Copy link
Contributor

@aaronbhansen this fix introduced other regressions, there would be many, many commits necessary to get the complete fix, as each attempt introduced a new regression :trollface: I'm not sure it is a good idea for us to attempt to backport this one given there is a relatively easy app-side fix:

let newRecord = store.createRecord(type, props);
newRecord.setProperties(relationships);

@aaronbhansen
Copy link

aaronbhansen commented Aug 14, 2018

I'll see if that works and update our app code for now. Thanks for the quick response

Update: that did fix the issue for anyone that might be experiencing it on 2.18

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.

5 participants