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.

Sign up for GitHub

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

Jump to bottom

[WARNING] Warn when extending JSONAPISerializer with extractMeta #4603

Merged
merged 1 commit into from
Oct 23, 2016
Merged

[WARNING] Warn when extending JSONAPISerializer with extractMeta #4603

merged 1 commit into from
Oct 23, 2016

Conversation

wecc
Copy link
Contributor

@wecc wecc commented Oct 20, 2016

No description provided.

@wecc wecc mentioned this pull request Oct 20, 2016
Closed
pangratz
pangratz reviewed Oct 20, 2016
View reviewed changes
addon/serializers/json-api.js
@@ -93,6 +93,39 @@ var dasherize = Ember.String.dasherize;

to the format that the Ember Data store expects.

**Customizing meta**
Copy link
Member

Choose a reason for hiding this comment

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

If you make this ## Customizing meta we should be able to directly link to this section in the warning below via http://emberjs.com/api/data/classes/DS.JSONAPISerializer.html#customizing-meta

urbany
urbany reviewed Oct 20, 2016
View reviewed changes
addon/serializers/json-api.js
export default JSONAPISerializer.extend({

normalizeArrayResponse(store, primaryModelClass, payload, id, requestType) {
let normalizedDocument = return this._super(...arguments);
Copy link

Choose a reason for hiding this comment

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

Good example. Just a nitpick: shouldn't have a return here, should be:
let normalizedDocument = this._super(...arguments);

Same thing for extractRelationship

@wecc wecc force-pushed the json-api-extractMeta-warning branch 2 times, most recently from 15413a2 to 58dcd98 Compare October 21, 2016 13:15
@bmac
Copy link
Member

bmac commented Oct 21, 2016

Hey @wecc do you mind amending the commit message and prefixing the commit with [WARNING]? This isn't a commit tag we are currently using but I'd like to try it out as an experiment to help us remember to mention this warning when we scan the changelog for things to mention in the release blog post.

@bmac
Copy link
Member

bmac commented Oct 21, 2016

Overall this change looks good to me @wecc 👍

@wecc wecc force-pushed the json-api-extractMeta-warning branch from 58dcd98 to 1be86c9 Compare October 21, 2016 19:53
@wecc wecc changed the title Warn when extending JSONAPISerializer with extractMeta [WARNING] Warn when extending JSONAPISerializer with extractMeta Oct 21, 2016
@wecc
Copy link
Contributor Author

wecc commented Oct 21, 2016

@bmac Sounds 👍 PR/commit updated!

@pangratz
Copy link
Member

There's a ' missing in https://github.com/emberjs/data/pull/4603/files#diff-c939dc9dc1a354cd6a97bd2c4ab65333R785

Wanna fix that in this PR too?

@wecc wecc force-pushed the json-api-extractMeta-warning branch from 1be86c9 to bdf8556 Compare October 21, 2016 20:03
@wecc
Copy link
Contributor Author

wecc commented Oct 21, 2016

@pangratz done!

@pangratz pangratz merged commit 6f53128 into emberjs:master Oct 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urbany urbany urbany left review comments

@pangratz pangratz pangratz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wecc @bmac @pangratz @urbany