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

remove useless private api #4654

Merged
merged 1 commit into from
Nov 18, 2016
Merged

remove useless private api #4654

merged 1 commit into from
Nov 18, 2016

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Nov 15, 2016

This follows #4637
cc @bmac

@param {DS.Model} record the record to serialize
@param {Object} options an options hash
*/
serialize(record, options) {
Copy link
Member

Choose a reason for hiding this comment

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

@sly7-7 do you mind deprecating this instead of removing it? Just in case someone was using it.

@pangratz
Copy link
Member

pangratz commented Nov 17, 2016

Since this is a public facing change, I think this should be put behind a feature flag 🤔

@sly7-7 can you update this PR to also include a test for the deprecation? Merci beaucoup!

@sly7-7
Copy link
Contributor Author

sly7-7 commented Nov 18, 2016

@pangratz unless I'm missing something, this method is marked @private, so there is no need to feature flag. I will add the test 😄 , I was too lazy to add one, considering the nature of the change.

@pangratz
Copy link
Member

Basically you're right. But since we're introducing a public facing deprecation, I think it would be good to put this behind a feature flag. If we - despite being private - see that too many applications use this in the wild, we can simply disable the feature.

I think the goal is to use feature flags for all public facing changes, even if they are very small.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Nov 18, 2016

feature flag and test added 😄

@param {DS.Model} record the record to serialize
@param {Object} options an options hash
*/
serialize(record, options) {
if (isEnabled('ds-deprecate-store-serialize')) {
deprecate('Use of serialize is deprecated, use record.serialize instead.', false, {
Copy link
Member

Choose a reason for hiding this comment

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

Can we be more explicit here and use Use of store.serialize is deprecated, use ... instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no pb 😄

@@ -146,3 +146,6 @@ entry in `config/features.json`.
}
});
```
- `ds-deprecate-store-serialize` [#4654](https://github.com/emberjs/data/pull/4654)

Adds a deprecation warning when using Store#serialize method
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add a short sentence on what should be used instead. Just so people looking at the FEATURES.md immediately know what this is about.

Copy link
Member

@pangratz pangratz left a comment

Choose a reason for hiding this comment

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

Nice, thanks @sly7-7 for the quick response! Just one more tiny nitpick, then prefixing this commit with [FEATURE ds-deprecate-store-serialize] and this is ready to go 🚀

@sly7-7
Copy link
Contributor Author

sly7-7 commented Nov 18, 2016

@pangratz I'm a little bit 😞 this takes so much reviews. I hope this is ok now 😄

@pangratz pangratz merged commit ef34f66 into emberjs:master Nov 18, 2016
@pangratz
Copy link
Member

🎉

@sly7-7 sly7-7 deleted the simplify-serialize-entry-points branch November 18, 2016 13:37
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.

None yet

4 participants