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

Fix EDMF for use with >3.2 #310

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

rondale-sc
Copy link
Contributor

The getRecords function inside Ember Data treats attributes
differently.

Where once the it would set each attribute on the internal model with
options[]= (triggering the setter/getter we install to properly
install EDMF) it now calls InternalModel#setDirtyAttribute.

The deciding factor for whether something is an attribute was based
off the meta set on the installed CP's. We're instead telling the
InternalModel#fields function to properly recognize fragments as
"fragments" which bypasses the setDirtyAttribute call and instead
does the default behavior from before.

Reopening the Model in this way preserves the previous behavior.

I suspect, this will likely need a large refactor when RFC293 lands

https://github.com/igorT/rfcs/blob/17e8f97049452ae18d3b12ee23c4858a6d5e0bfd/text/0000-model-data.md

Luckily, I believe IgorT is already begun the work there with this spike
listed in the above RFC:

https://github.com/igorT/ember-data.model-fragments/tree/igor/model-data

The `getRecords` function inside Ember Data treats attributes
differently.

Where once the it would set each attribute on the internal model with
`options[]=` (triggering the setter/getter we install to properly
install EDMF) it now calls `InternalModel#setDirtyAttribute`.

The deciding factor for whether something is an `attribute` was based
off the `meta` set on the installed CP's.  We're instead telling the
`InternalModel#fields` function to properly recognize fragments as
`"fragments"` which bypasses the `setDirtyAttribute` call and instead
does the default behavior from before.

Reopening the Model in this way preserves the previous behavior.

I suspect, this will likely need a large refactor when RFC293 lands

https://github.com/igorT/rfcs/blob/17e8f97049452ae18d3b12ee23c4858a6d5e0bfd/text/0000-model-data.md

Luckily, I believe IgorT is already begun the work there with this spike
listed in the above RFC:

https://github.com/igorT/ember-data.model-fragments/tree/igor/model-data
@rondale-sc
Copy link
Contributor Author

The specific part we are overriding to do the default behavior is this switch statement:

https://github.com/emberjs/data/blob/d41601529a09636e636524a6d80b4d877952c344/addon/-legacy-private/system/model/internal-model.js#L381

@jakesjews
Copy link
Contributor

@rondale-sc awesome it's so nice seeing a green build! @workmanw hows this look to you?

@rwjblue
Copy link
Member

rwjblue commented Aug 1, 2018

Seems reasonable to me (FWIW)...

@workmanw workmanw self-requested a review August 1, 2018 11:45
@workmanw
Copy link
Contributor

workmanw commented Aug 1, 2018

@rondale-sc I assume the model-fragment tests were broken with ember-data 3.2 prior to this fix? Just want to make sure we have some kind of test coverage.

@rwjblue
Copy link
Member

rwjblue commented Aug 1, 2018

Yes, a number of tests failed before this PR when ran against [email protected]+.

@rondale-sc
Copy link
Contributor Author

@workmanw @rwjblue Yeah, what Robert said is correct. Specifically we had some tests around fragments being handled when a POJO with a fragment is passed to createRecord (which was what started me on this path).

@jakesjews jakesjews merged commit 0c67920 into adopted-ember-addons:master Aug 1, 2018
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.

4 participants