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

[Docs] DS.RESTSerializer needs update #4186

Closed
James1x0 opened this Issue Feb 25, 2016 · 12 comments

Comments

Projects
None yet
4 participants
@James1x0
Copy link
Contributor

James1x0 commented Feb 25, 2016

normalizeHash is deprecated, Parse is going extinct.

@bmac

This comment has been minimized.

Copy link
Member

bmac commented Feb 26, 2016

For some context: normalizeHash and Parse are referenced on this page
http://emberjs.com/api/data/classes/DS.RESTSerializer.html which is auto generated from the API docs in this file https://github.com/emberjs/data/blob/master/addon/serializers/rest.js.

@James1x0

This comment has been minimized.

Copy link
Contributor Author

James1x0 commented Feb 26, 2016

Sorry @bmac, had to make a quick issue regarding this so I wouldn't forget. I may have time to tackle this soon. May not tho.

@jsangilve

This comment has been minimized.

Copy link
Contributor

jsangilve commented Mar 8, 2016

@James1x0 I've just realized that normalizeHash is deprecated. Apart from docs, it seems like no deprecation warning is thrown when you use it. I'm using it with Ember Data 2.3

@James1x0

This comment has been minimized.

Copy link
Contributor Author

James1x0 commented Mar 8, 2016

@jsangilve It threw a warning in ED 1.x, I think it's supposed to be completely gone from the api in 2.x

@jsangilve

This comment has been minimized.

Copy link
Contributor

jsangilve commented Mar 13, 2016

@James1x0 it's gone from the API, but it seems like the normalize method of DS.RESTSerializer still uses a normalizeHash property:

normalize(modelClass, resourceHash, prop) {
    if (this.normalizeHash && this.normalizeHash[prop]) {
      this.normalizeHash[prop](resourceHash);
    }
    return this._super(modelClass, resourceHash, prop);
  },

If you create a normalizeHash property while extending DS.RESTSerializer, it will work. I just created a PR that only solves the docs issue.

@pangratz

This comment has been minimized.

Copy link
Member

pangratz commented Mar 14, 2016

comment by @jsangilve

I've just realized that normalizeHash is deprecated. Apart from docs, it seems like no deprecation warning is thrown when you use it. I'm using it with Ember Data 2.3

There is a deprecation warning in 1.13, but it looks like there has been something overlooked when the deprecation has been removed in 2.0.


Since it has been deprecated in 1.13, the normalizeHash stuff should be removed from within normalize() of the rest serializer; this eventually makes the normalize() obsolete in that serializer since it would only call _super().

Unfortunately this would be a breaking change since this seems to be used, as pointed out by @jsangilve. There is also a test for this. So I think the correct way would be to deprecate this path again and remove it in 3.0 😢

@jsangilve

This comment has been minimized.

Copy link
Contributor

jsangilve commented Mar 14, 2016

@pangratz I agree.

I can open a new PR to include the deprecation message and new tests without normalizeHash. I think it should be some like:

normalize(modelClass, resourceHash, prop) {
  if (this.normalizeHash) {
    Ember.deprecate('`RESTSerializer.normalizeHash` has been deprecated. Please use `serializer.normalize` to modify the payload of single resources.');

    if (this.normalizeHash[prop]) {
      this.normalizeHash[prop](resourceHash);
    }
  }
  return this._super(modelClass, resourceHash);
},
@pangratz

This comment has been minimized.

Copy link
Member

pangratz commented Mar 14, 2016

@jsangilve I will check the next steps here and get back to you regarding that PR.

But basically it would be the same as here (also with id and until in the passed hash). Also the test I mentioned would need to be updated so the deprecation message is checked via assert.expectDeprecation(). I will let you know once the path is clear. Thanks for your help on this! 🚀

bmac added a commit that referenced this issue Mar 22, 2016

Merge pull request #4228 from jsangilve/master
Solves #4186 and fixes `this._super` call within `normalize` method.
@pangratz

This comment has been minimized.

Copy link
Member

pangratz commented Mar 22, 2016

@jsangilve I am very sorry, but I completely forgot to get back to you here 😢 In the meantime this has already been addressed in #4258.

@jsangilve

This comment has been minimized.

Copy link
Contributor

jsangilve commented Mar 22, 2016

No problem @pangratz. Just let me know if the deprecation path discussed above is approved. I'll be more than glad to PR with new tests.

@jsangilve

This comment has been minimized.

Copy link
Contributor

jsangilve commented Mar 22, 2016

@pangratz Oh, now I see it has been already fixed. Awesome!

@pangratz

This comment has been minimized.

Copy link
Member

pangratz commented May 10, 2016

Closing this as the normalizeHash has been re-deprecated in #4258 and the docs have been updated in #4228. The API docs should be updated once the next minor release is cut.

@pangratz pangratz closed this May 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment