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

[DEPRECATION] RFC-522 implementation to add deprecations #6390

Merged
merged 23 commits into from
Sep 18, 2019

Conversation

pete-the-pete
Copy link
Contributor

@pete-the-pete pete-the-pete commented Aug 29, 2019

RFC-522 implementation to add deprecations for default serializers and adapters.

NOTE: this is an initial commit; refinements and test changes coming

Deprecation Docs: ember-learn/deprecation-app#436

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you update the deprecate calls here to include a url property that links to the deprecation guides that you are working to add over in https://github.com/ember-learn/deprecation-app/tree/master/content/ember-data/v3?

I think it should also add some deprecation tests (to ensure the deprecations are fired in the right circumstances).

@pete-the-pete
Copy link
Contributor Author

pete-the-pete commented Aug 29, 2019 via email

@pete-the-pete
Copy link
Contributor Author

Small update to add deprecatedTest and remove some deprecations.

@rwjblue I didn't see other deprecations with the url prop, can you point me to an example?
I'm on vacation now, but will have time to work on this a little over the next few days.

@rwjblue
Copy link
Member

rwjblue commented Sep 2, 2019

Here is an example from ember-source:

https://github.com/emberjs/ember.js/blob/f73d8440d19cf86a10c61ddb89d45881acfcf974/packages/%40ember/-internals/console/index.js#L44-L48

The Ember Inspector, the deprecation workflow, and the Ember.deprecate console logging shows this URL when the deprecation is triggered. It helps folks know what to do (and understand the deprecation a bit more).

@pete-the-pete
Copy link
Contributor Author

Here is an example from ember-source:

https://github.com/emberjs/ember.js/blob/f73d8440d19cf86a10c61ddb89d45881acfcf974/packages/%40ember/-internals/console/index.js#L44-L48

The Ember Inspector, the deprecation workflow, and the Ember.deprecate console logging shows this URL when the deprecation is triggered. It helps folks know what to do (and understand the deprecation a bit more).

@rwjblue I added links to the docs I created here: ember-learn/deprecation-app#436

@runspired runspired added the 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 label Sep 11, 2019
@pete-the-pete pete-the-pete force-pushed the RFC-522-implementation branch 2 times, most recently from 752e22d to 5f351dc Compare September 15, 2019 20:54
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Overall looks good on the implementation but the descriptions need some more work

packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/model/model.js Outdated Show resolved Hide resolved
@pete-the-pete pete-the-pete force-pushed the RFC-522-implementation branch 3 times, most recently from 34534e2 to b89df01 Compare September 18, 2019 01:32
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Almost there! :)

packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/model/model.js Outdated Show resolved Hide resolved
Copy link
Member

rwjblue commented Sep 18, 2019

I just landed ember-learn/deprecation-app#436 (thank you for the hard work!), can you confirm that the published deprecation URLs match the ones we have here?

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Added updates for URLs

packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/core-store.ts Outdated Show resolved Hide resolved
packages/store/addon/-private/system/model/model.js Outdated Show resolved Hide resolved
pete-the-pete and others added 4 commits September 18, 2019 15:37
Co-Authored-By: Chris Thoburn <runspired@users.noreply.github.com>
Co-Authored-By: Chris Thoburn <runspired@users.noreply.github.com>
Co-Authored-By: Chris Thoburn <runspired@users.noreply.github.com>
Co-Authored-By: Chris Thoburn <runspired@users.noreply.github.com>
@pete-the-pete
Copy link
Contributor Author

Added updates for URLs

🙇

@runspired runspired merged commit 4551d05 into emberjs:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants