-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[FEAT] - add a public Typescript interface for the minimal required serializer interface #6113
Conversation
155bbc3
to
f7527a5
Compare
export default interface Serializer { | ||
store: any; | ||
normalize(typeClass: any, hash: any): any; | ||
normalizeResponse(store: any, primaryModelClass: any, payload: JsonApiResource, id: string | number, requestType: string): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the minimal serializer need to have both normalize
and normalizeResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@runspired @igorT seems like a good opportunity to nix normalize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store.normalize
is our only non-serializer code that calls Serializer.normalize
, agreed that we should nix it, would involve deprecating Store.normalize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean I should remove it from this? or add the deprecation to Store.normalize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My view is that we should nix this; the serializer interface should have only normalizeResponse
and serialize
@runspired is quite right that we should separately deprecate the other hooks.
We should add tests to verify that you can write a userland serializer that implements only those two hooks.
I don't know. I went off the interface of
https://api.emberjs.com/ember-data/3.10/classes/DS.Serializer
…On Wed, May 15, 2019 at 2:25 PM David J. Hamilton ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/store/addon/-private/ts-interfaces/serializer.ts
<#6113 (comment)>:
> @@ -0,0 +1,9 @@
+import {
+ JsonApiResource
+} from './record-data-json-api';
+export default interface Serializer {
+ store: any;
+ normalize(typeClass: any, hash: any): any;
+ normalizeResponse(store: any, primaryModelClass: any, payload: JsonApiResource, id: string | number, requestType: string): any;
Why would the minimal serializer need to have both normalize and
normalizeResponse?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AACUPANP57RFKITYH7JUOQLPVR5U7A5CNFSM4HM7WX62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBYYM2ZQ#pullrequestreview-238079334>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACUPAOFDITZVO2IEH5KXETPVR5U7ANCNFSM4HM7WX6Q>
.
|
We may also have to include |
f7527a5
to
695d30d
Compare
Closing in favor of #6451 |
@runspired is this even close? Looking at some other TS interface definitions, is seems like there are a few layers of definitions, in this case DS.Store, DS.Model. Should those be defined as well?
How should this be tested?