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

Deprecate volatile option #718

Merged

Conversation

fsmanuel
Copy link
Contributor

@fsmanuel fsmanuel commented Aug 17, 2022

Ember 4.0 removed .volatile() on computed(). This PR adds the deprecation to remove it in version 5.0 of this addon.

Tests for ember-4.0, release, beta, and canary are expected to fail.

@fsmanuel fsmanuel mentioned this pull request Aug 17, 2022
14 tasks
@fsmanuel
Copy link
Contributor Author

@MelSumner before we release a new beta version we should merge this to get the deprecation out.

@RobbieTheWagner
Copy link
Member

@fsmanuel we should just remove this entirely. I wouldn't worry about deprecating it. We should update all the things to work with latest Ember and not worry about supporting legacy Ember versions IMO.

@fsmanuel
Copy link
Contributor Author

@rwwagner90 In general I agree. The thing with this addon is that it should probably not be used in new ember apps as the main mental model is about computed properties. In the projects I currently use it I don't know when to migrate to ember version >= 4.0 but I would like to get rid of the super annoying deprecations. That is why I think we should have a version that works for ember < 4.0
The other argument is that we can not just remove features (the volatile option is a feature) just to make it work in new ember versions without thinking about a replacement.

I'm already working on the ember 4.0 version: #717

@RobbieTheWagner
Copy link
Member

@fsmanuel yes, but volatile has been deprecated a very long time in Ember. I think we can remove it. Those who want to still use it can just use earlier versions of this addon.

@knownasilya
Copy link
Contributor

Agreed. Could you remove it @fsmanuel? I can merge and release once that's done.

@MelSumner
Copy link
Contributor

@knownasilya I agree with removing it instead of adding a deprecation notice, but should we get it working for 4.0 before doing a new release? WDYT?

@knownasilya
Copy link
Contributor

I meant the next version for the beta. I think there was a few things still left to remove the beta flag.

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Aug 19, 2022

@rwwagner90 @MelSumner @knownasilya My point is that volatile is a documented option/feature of the addon and if we remove it things will break without a notice. So I thought we deprecate it and add an assert when we remove it.

If you disagree I'm fine with it and we can close the PR and release a new beta version with current master.

I'll continue working on #717 and drop volatile there.

@knownasilya
Copy link
Contributor

@fsmanuel yeah. Let's do that. We can do multiple major versions if we need.

@knownasilya knownasilya merged commit 80f6dca into adopted-ember-addons:master Aug 22, 2022
@fsmanuel fsmanuel deleted the deprecate-volatile branch August 23, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants