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

Refactor the Serializer API #3143

Merged
merged 6 commits into from
Jun 5, 2015
Merged

Refactor the Serializer API #3143

merged 6 commits into from
Jun 5, 2015

Conversation

wecc
Copy link
Contributor

@wecc wecc commented Jun 2, 2015

This PR refactors the way serializers normalizes adapter responses. serializer.extract() is now deprecated for the new serializer.normalizeResponse().

normalizeResponse() is expected to return a JSON-API Document with primary and, if any, secondary/sideloaded/included as { data, included }.

normalize() is also expected to return a JSON-API Document having the primary data be the normalized record in { data } and, if any, secondary data (e.g. embedded) in { included }.

This change should be fully backwards compatible. For users to opt-in to the new Serializer API they should convert their existing extract() hooks to normalizeResponse() hooks and make it return JSON-API. Then add isNewSerializerAPI: true in their serializer(s).

Todos

  • Rename REST2Serializer, EmbeddedRecords2Mixin and ActiveModel2Serializer to something that makes sense
  • Figure out what and how to deprecate the old API
  • Feature flag / set isNewSerializerAPI for non-extended serializers
  • Create hooks to make it easier to customize normalizeResponse()
  • Break up normalizeResponse() to reflect the old extractArray/extractSingle methods
  • Add deprecation notices

@igorT
Copy link
Member

igorT commented Jun 2, 2015

@wecc you forgot to mention JSON2Serializer

@sly7-7
Copy link
Contributor

sly7-7 commented Jun 2, 2015

I'm probably missing something, but why not naming those classes as JSONAPIXxx ?

@bmac
Copy link
Member

bmac commented Jun 2, 2015

I think the fear is that JSONAPI in the name will be confusing because the new "2" serializers are not intended to be used with a JSONAPI backend. They simply transform a non JSONAPI data into JSON API format, with the eventual goal of Ember Data using json api natively, allowing 3rd party serializer authors to target JSON API instead of the unspecified "normalized" format that Ember Data uses today.

@sly7-7
Copy link
Contributor

sly7-7 commented Jun 2, 2015

@bmac Thanks, that makes sense


function coerceId(id) {
return id == null ? null : id + '';
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this method is repeated in the rest2-serializer rest-serializer and store module. Lets move it into its own file and import it in all 3 places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@wecc wecc force-pushed the serializer-refactor branch 3 times, most recently from 3d81d51 to 32072fd Compare June 2, 2015 21:38
@bmac bmac added this to the v1.0.0-beta.19 milestone Jun 3, 2015
@wecc wecc force-pushed the serializer-refactor branch 3 times, most recently from cbb1a47 to 5c93d60 Compare June 4, 2015 19:56
return this.normalizeDeleteRecordResponse(...arguments);
case 'updateRecord':
return this.normalizeUpdateRecordResponse(...arguments);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default to return this._normalizeResponse(...arguments)

Copy link
Member

Choose a reason for hiding this comment

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

could also dry this up to this['normalize' + capitalize(requestType)] || this._normalizeResponse)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this vague memory that we wanted to change this to switch to make it easier for people to see what actual requestTypes are getting passed. We just never got around to change it here. The discussion was originally around https://github.com/emberjs/data/blob/master/packages/ember-data/lib/adapters/build-url-mixin.js#L52-L75

@wecc wecc changed the title WIP - Serializer refactor WIP - Refactor the Serializer API Jun 4, 2015
@igorT igorT mentioned this pull request Jun 4, 2015
28 tasks
@wecc wecc force-pushed the serializer-refactor branch 2 times, most recently from 1028aa6 to 5fac1e8 Compare June 5, 2015 00:33
@wecc wecc changed the title WIP - Refactor the Serializer API Refactor the Serializer API Jun 5, 2015
@igorT
Copy link
Member

igorT commented Jun 5, 2015

👍 on structuring the commits

},

/**
Returns the resource's relationships formatted as an JSON-API "relationships object".
Copy link
Member

Choose a reason for hiding this comment

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

s/an/a/

@wecc wecc force-pushed the serializer-refactor branch 2 times, most recently from a02a530 to 053f227 Compare June 5, 2015 19:25

var ids = [];
hash[key] = embeddedRecord.id;
//TODO Need to add a reference to the parent later so relationship works between both `belongsTo` records
Copy link
Member

Choose a reason for hiding this comment

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

Don't need this todo anymore

@igorT
Copy link
Member

igorT commented Jun 5, 2015

👍

@wecc wecc force-pushed the serializer-refactor branch 2 times, most recently from b8a25ae to e3c3796 Compare June 5, 2015 20:15
wecc added a commit that referenced this pull request Jun 5, 2015
@wecc wecc merged commit 4f0176f into master Jun 5, 2015
@wecc wecc deleted the serializer-refactor branch September 24, 2015 21:26
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.

6 participants