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

[DOC] Update embedded-records-mixin.js #6958

Merged
merged 1 commit into from
Feb 27, 2020
Merged

[DOC] Update embedded-records-mixin.js #6958

merged 1 commit into from
Feb 27, 2020

Conversation

allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Jan 10, 2020

Adding documentation on the embedded records mixin explaining that it does not work with JSONAPISerializer. This should be noted more prominently rather than in a passive warning message (which might get buried under a sea of deprecation warnings in the test suite).

@@ -19,6 +19,8 @@ import { warn } from '@ember/debug';

Note that embedded records will serialize with the serializer for their model instead of the serializer in which they are defined.

Note also that this mixin does not work with JSONAPISerializer because the JSON API spec does not describe how to format embedded resources.
Copy link

Choose a reason for hiding this comment

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

General question I do not know the answer to: what versions of the JSON:API spec does ember-data adhere to? I would like to qualify this caveat with that answer.

Copy link
Contributor Author

@allthesignals allthesignals Jan 13, 2020

Choose a reason for hiding this comment

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

image

edit: tagging @NullVoxPopuli so he knows I screen-shotted his discord message :P

Copy link

Choose a reason for hiding this comment

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

Thanks! Upon further reflection I think ember-data should describe its compatibility with JSON:API somewhere for transparency. It seems better suited to top-level documentation or some kind of introduction. So we can leave it out of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Upon further reflection I think ember-data should describe its compatibility with JSON:API somewhere for transparency. It seems better suited to top-level documentation or some kind of introduction.

We do in the serializer documentation overview

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Caveat is a good idea; Could you update the language to be more formal: JSON API spec to JSON:API specification? Thanks!

@ghost
Copy link

ghost commented Jan 15, 2020

@runspired any objections to merging? Also, I think we can add the changelog:doc label.

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.

Can you change the target branch to be master (we typically don't land PR's into release branch)?

@allthesignals allthesignals changed the base branch from release to master January 23, 2020 03:59
@allthesignals allthesignals requested a review from rwjblue January 23, 2020 04:14
@hjdivad hjdivad changed the title Update embedded-records-mixin.js [DOC] Update embedded-records-mixin.js Feb 19, 2020
@hjdivad
Copy link
Member

hjdivad commented Feb 19, 2020

rebased which should get CI green

@MelSumner
Copy link
Contributor

@hjdivad looks like a few ember data tests are still failing- any insight there?

Embedded Records Mixin is not compatible with JSONAPISerializer
@allthesignals
Copy link
Contributor Author

Okay, I went ahead and force-pushed a change with a fresh branch.

I'm not sure why but simply rebasing this branch triggered a few merge conflicts with unrelated files. Any theories on why that might've been happening would be helpful for me in the future :)

@hjdivad hjdivad merged commit 648f2f5 into emberjs:master Feb 27, 2020
@hjdivad
Copy link
Member

hjdivad commented Feb 27, 2020

<3 Thanks @allthesignals !

@hjdivad
Copy link
Member

hjdivad commented Feb 27, 2020

@hjdivad looks like a few ember data tests are still failing- any insight there?

Those were failing external partner tests.

In particular travis-web: Acceptance | repo/trigger build: triggering a custom build via the dropdown
. @runspired @igorT is this a test with a race condition or something? I recall seeing this crop up a lot in false negatives.

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