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 EmbeddedRecordsMixin] Assert from JSONAPISerializer #7167

Merged
merged 1 commit into from
May 12, 2020

Conversation

allthesignals
Copy link
Contributor

Use of EmbeddedRecordsMixin and JSONAPISerializer together asserts instead of warns. Allows for setting "isEmbeddedRecordsMixinCompatible" on the serializer for when parts of the API are compatible.

Closes #6991

'The JSONAPISerializer does not work with the EmbeddedRecordsMixin because the JSON API spec does not describe how to format embedded resources.',
!props.isEmbeddedRecordsMixin,
!props.isEmbeddedRecordsMixin || props.isEmbeddedRecordsMixinCompatible === true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably want a test for allowing this

Copy link
Member

Choose a reason for hiding this comment

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

yep, also mention the opt-in flag in the assertion message

'The JSONAPISerializer does not work with the EmbeddedRecordsMixin because the JSON API spec does not describe how to format embedded resources.',
!props.isEmbeddedRecordsMixin,
!props.isEmbeddedRecordsMixin || props.isEmbeddedRecordsMixinCompatible === true,
Copy link
Member

Choose a reason for hiding this comment

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

yep, also mention the opt-in flag in the assertion message

@hjdivad
Copy link
Member

hjdivad commented May 6, 2020

thanks for taking a look at this @allthesignals!

@allthesignals
Copy link
Contributor Author

allthesignals commented May 11, 2020

Looking at this again tonight, but I'm a bit stuck.

I need a way to assert based on values from both the mixed-in class and the mixin itself (check isEmbeddedRecordsMixin on the mixin and isEmbeddedRecordsMixinCompatible on the user class extension).

Right now, the EmbeddedRecordsMixin assertion lives on the JSONAPISerializer itself, and is run with the willMergeMixin hook (which I can't find documentation for).

There are some other assertions sprinkled throughout the JSONAPISerializer class, so I think my best bet is to move the assertion check there, unless there is a better approach.

Alright, I just put it into init. Not sure if there's a problem with that approach.

@allthesignals allthesignals force-pushed the embedded-records-assertion branch 3 times, most recently from 5cdb116 to 318c410 Compare May 12, 2020 00:29
@hjdivad
Copy link
Member

hjdivad commented May 12, 2020

@runspired do we expect the IE11 asset size check to be this sensitive? it's complaining about a 50B (not Kb) increase.

Copy link
Member

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

LGTM assuming tests 🍏

@allthesignals
Copy link
Contributor Author

allthesignals commented May 12, 2020

Just guessing but maybe the inclusion of the code inside init means it doesn't get stripped out during some production build step? Might be why some of these other warns are added into the reopen inside the debug check.

Oh, interesting — the assertion does get stripped out but leaves the empty init:

init:function(){this._super.apply(this,arguments)}

And it's exactly 50 bytes.

Edit: Oh. I can just put it inside that reopen call and I think it'll get stripped out.

Use of EmbeddedRecordsMixin and JSONAPISerializer together asserts instead of warns. Allows for setting "isEmbeddedRecordsMixinCompatible" on the serializer for when parts of the API are compatible.
@allthesignals allthesignals force-pushed the embedded-records-assertion branch from 318c410 to 7b01137 Compare May 12, 2020 01:13
@hjdivad hjdivad merged commit ad202bf into emberjs:master May 12, 2020
@hjdivad
Copy link
Member

hjdivad commented May 12, 2020

thanks @allthesignals!

@runspired
Copy link
Contributor

@hjdivad we do expect it to be that sensitive. The idea is that any regression is one we are acutely aware of, vs allowing lots of tiny drift to be added over time unnoticed.

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.

clarify support and behavior around embedded records mixin
3 participants