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

clarify support and behavior around embedded records mixin #6991

Closed
ghost opened this issue Jan 22, 2020 · 4 comments · Fixed by #7167
Closed

clarify support and behavior around embedded records mixin #6991

ghost opened this issue Jan 22, 2020 · 4 comments · Fixed by #7167

Comments

@ghost
Copy link

ghost commented Jan 22, 2020

Initial documentation was landed in #6958, but we should make it clearer for folks by default. After discussing in the ember-data core meeting on 2020-02-19, we (@hjdivad, @rwjblue, and @efx) propose:

Updating the embedded records mixin to assert when used with JSONAPISerializer unless a specific flag to opt out of the assertion is used.

Something like:

assert(
  'The embedded records mixin does not work with all JSON:API\'s, please confirm this works for your specific API and add `this.isEmbeddedRecordsMixinCompatible = true` to your serializer', 
  this.isJSONAPISerializer !== true || this.isEmbeddedRecordsMixinCompatible === true
);
@adriancerejo
Copy link

I am fairly new to open source contributions and was wondering if I could take this issue.

@allthesignals
Copy link
Contributor

allthesignals commented May 5, 2020

Hmm I think it might go here: https://github.com/emberjs/data/blob/v3.17.0/packages/serializer/addon/json-api.js

I'm not sure where makes the most sense, maybe the constructor?

EDIT: Oh, maybe here: https://github.com/emberjs/data/blob/v3.17.0/packages/serializer/addon/json-api.js#L560-L590

The original warning could be turned into an assert?

@hjdivad
Copy link
Member

hjdivad commented May 6, 2020

@allthesignals @adriancerejo changing the existing warning you identified to an assertion with an opt-out flag would be great.

@allthesignals
Copy link
Contributor

@hjdivad I've got a branch but I'm not sure which to PR into, gonna check docs in a sec :)

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 a pull request may close this issue.

3 participants