-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Clarify the way forward #155
Comments
Thanks @Turbo87 for keeping at this. My take on this:
|
my issue with that is that as soon as you install the addon, you don't have any deprecation warnings for |
True, however, this addon intends to provide the functionality itself and therefore no deprecation is needed. If folks want to migrate away from using I think one thing seems confusing amongst our collective opinions here though, and I'd like to clarify: @Turbo87 - I think you are viewing this addon as a stepping stone to eventually changing to
And later, it talks specifically about
tldr; I think you are asking for a new feature from this addon (which I'm not opposed to adding as an opt-in), which would continue to emit a deprecation when using |
why does the addon no longer provide |
Ya, true. The intent of that change (definitely a divergence from the RFC as well) was specifically to allow this addon to continue provide support for adding jQuery to the build while the |
that does not answer the question though. why does it not also provide support for IMHO we need to decide between either deprecating |
Part of the confusion here may stem from the fact that we haven't landed #31 yet (and won't until Ember deprecates its internal jQuery based event dispatcher). This addon isn't only going to be providing support for When |
FWIW it seems strange to me that we are in v1.x land already but have not implemented such a core part of the addon 🤔
I disagree. To me the stuff in |
It isn't implemented, because even if we did implement it and release it we'd be duplicating asset size with ember-source's version. Also, the implementation is largely done (between #31 and the code that exists in Ember itself). We will only land the code here in a release once |
Hmm, this can't be true actually. We absolutely expect addons (other than |
I think this is an excellent point. I don't have a good answer here. @simonihmig - Thoughts? |
So, looking at the addon story here specifically, the RFC is not very specific as to how having
It's true that this promise of the RFC is not fulfilled anymore, given the changes in #148. Especially since there is no warning about it anymore. So we could revert that change. On the other hand when adding an addon that depends on But at least we would have to show him a clear warning message if Either way, the idea that we can "reuse"
Not sure if you raised that concern here actually... so disabling the integration feature, which was perfectly working before without any deprecations, would lead to code still relying on its APIs to break. But that's pretty much what you would have to expect when disabling a feature that you rely on. And there is a direct correlation between the action (of disabling the feature) and the effect (stuff breaks), so not much room for confusion (I hope). So I wouldn't worry about that case. |
Agree. If a non-project depends on
Yeah, I suppose. Do we just tell folks to add an |
Following some discussions on Discord it is still not entirely clear what the way forward should be. I'm attempting to write down the current situation here and would welcome comment on how this is supposed to be changed:
@ember/jquery
is not usedjquery-integration
is not setjquery-integration: enabled
Ember.$
shows a deprecation forEmber.$
import $ from 'jquery';
shows a deprecation forEmber.$
this.$()
shows a deprecation forthis.$()
Ember.$
throws an errorimport $ from 'jquery';
throws an errorthis.$()
throws an error@ember/jquery
is usedEmber.$
??? (not tested)import $ from 'jquery';
works without deprecation warningthis.$()
shows a deprecation forthis.$()
Ember.$
throws an errorimport $ from 'jquery';
works without deprecation warningthis.$()
throws an error/cc @rwjblue @chancancode @simonihmig
The text was updated successfully, but these errors were encountered: