Clarify the way forward
Turbo87 opened this issue · 13 comments
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:
- if
@ember/jquery
is not used- if
jquery-integration
is not set- the build shows a warning about either setting the flag or using the addon, otherwise same situation as
jquery-integration: enabled
- the build shows a warning about either setting the flag or using the addon, otherwise same situation as
- if `jquery-integration is enabled
Ember.$
shows a deprecation forEmber.$
import $ from 'jquery';
shows a deprecation forEmber.$
this.$()
shows a deprecation forthis.$()
- if `jquery-integration is disabled
Ember.$
throws an errorimport $ from 'jquery';
throws an errorthis.$()
throws an error
- if
- if
@ember/jquery
is used- if `jquery-integration is enabled
Ember.$
??? (not tested)import $ from 'jquery';
works without deprecation warningthis.$()
shows a deprecation forthis.$()
- if `jquery-integration is disabled
Ember.$
throws an errorimport $ from 'jquery';
works without deprecation warningthis.$()
throws an error
- if `jquery-integration is enabled
Thanks @Turbo87 for keeping at this. My take on this:
-
if
@ember/jquery
is not used
should be fine as described above -
if
@ember/jquery
is used- if
jquery-integration
is enabledEmber.$
shows a deprecation forEmber.$
- which should be the case alreadyimport $ from 'jquery';
works without deprecation warningthis.$()
should work and show no deprecations
- if
jquery-integration
is disabled
should be fine as described above
- if
my issue with that is that as soon as you install the addon, you don't have any deprecation warnings for this.$()
anymore, which would help you to at some point disable the jquery-integration
flag.
my issue with that is that as soon as you install the addon, you don't have any deprecation warnings for this.$() anymore
True, however, this addon intends to provide the functionality itself and therefore no deprecation is needed.
If folks want to migrate away from using jquery-integration: true
, we can either add an opt-in way to avoid squelching the deprecation or we could focus on linting against this.$
in Ember.Component
's.
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 jquery-integration: false
, but that is not a goal of this addon. Here are a few relevant quotes from the RFC (which was the reason this addon was created):
This RFC does not propose to discourage the use of jQuery. There are legitimate cases where you still want to have it. And this is also true for addons, especially those that basically wrap other jQuery-based libraries like jQuery plugins in an Ember friendly way. For those cases, there should be an opt-in path to continue bundling jQuery and to preserve the existing APIs around it. This is what the @ember/jquery package is meant for.
And later, it talks specifically about this.$
:
RFC294 already introduced this package, being responsible to include jQuery into the JavaScript bundle. As part of this RFC the scope of this addon will be extended to also reintroduce the deprecated APIs, but without triggering any deprecation warnings for this.$() in a component.
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 this.$
in components.
For those cases, there should be an opt-in path to continue bundling jQuery and to preserve the existing APIs around it. This is what the @ember/jquery package is meant for.
why does the addon no longer provide this.$()
when the jquery-integration
flag is disabled? seem contrary to the above statement.
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 jquery-integration
flag was off. In a future world with Embroider / ember-auto-import built in, we likely don't need this addon to provide that build time support. But for now, it seemed like the easier path to take...
that does not answer the question though. why does it not also provide support for this.$()
in that case?
IMHO we need to decide between either deprecating this.$()
here too or providing it unconditionally. not deprecating it, but then also not providing it if the flag is disabled makes no sense to me.
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 this.$
when the flag is enabled, but it will also be how you would continue using jQuery based event dispatching through Ember 4.x.
When jquery-integration: false
is set, the only reason to use this addon is to enable an app to add jQuery to its build. If the app wants to continue to use jquery-integration: true
, there is no reason to issue a deprecation for this.$
/jQuery-EventDispatcher.
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
When jquery-integration: false is set, the only reason to use this addon is to enable an app to add jQuery to its build.
I disagree. To me the stuff in optional-features.json
is bound to the ember-source
package, or maybe even ember-cli
, but should not influence any other addons. We recommend in the deprecation guide that addons that rely on jQuery use this addon here to force apps into providing jQuery and this.$()
to them. But if the app disables jquery-integration
that suddenly won't be true anymore. But you also wouldn't have gotten any deprecation warnings because using @ember/jquery
means you won't get any deprecations.
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
🤔
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 ember-source
is able to avoid shipping it.
To me the stuff in
optional-features.json
is bound to the ember-source package, or maybe even ember-cli, but should not influence any other addons.
Hmm, this can't be true actually. We absolutely expect addons (other than ember-source
) to check for optional feature values, this is why we have added an explicit API to @ember/optional-features
for them to be able to interrogate the state of those optional flags (project.addons.findAddonByName('@ember/optional-features').isFeatureEnabled('jquery-integration')
). I definitely agree (in general) that the primary focus of config/optional-features.json
is opting in and out of features provided by ember-source
, but thats quite different from "should not influence other addons" IMHO.
We recommend in the deprecation guide that addons that rely on jQuery use this addon here to force apps into providing jQuery and this.$() to them. But if the app disables jquery-integration that suddenly won't be true anymore. But you also wouldn't have gotten any deprecation warnings because using @ember/jquery means you won't get any deprecations.
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 @ember/jquery
relates to the integration flag. I guess this gives us the opportunity to figure out what makes most sense, without violating the text of the RFC. I think there are two issues mentioned here:
- Addons that need jQuery should continue to work by depending on
@ember/jquery
We recommend in the deprecation guide that addons that rely on jQuery use this addon here to force apps into providing jQuery and this.$() to them.
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 @ember/jquery
silently forces the app to go into jQuery integration mode, that could also be confusing, as especially the (later to be introduced by #31) jQuery-based EventDispatcher could introduce subtle bugs in the app (differences between jQuery events and native ones). So maybe just having @ember/jquery
in an addon's dependency is not enough, instead we expect the user to explicitly turn the integration flag on to force him to make that call?
But at least we would have to show him a clear warning message if @ember/jquery
is present but the integration is not turned on. As it was before that latest PR, maybe with a different message when it is a transitive dependency (i.e. coming from an addon). Something along the lines of You have @ember/jquery in the dependencies of ember-foo, but the jquery-integration optional feature is not turned on. Enable the optional feature to make the addon work as expected. Note that this will change the way Ember events are dispatched in your app.
.
Either way, the idea that we can "reuse" @ember/jquery
to just bundle jQuery without doing the Ember integration seems eventually wrong? As you wouldn't want to see any warnings for that other legit use case. So maybe the bundling-only use case should be addresses by a separate (user-land?) addon like ember-jquery
? (#1). Or just refer to Embroider/auto-import?
- What happens when the integration flag was enabled, but then gets disabled
But if the app disables jquery-integration that suddenly won't be true anymore. But you also wouldn't have gotten any deprecation warnings because using @ember/jquery means you won't get any deprecations
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.
So maybe just having @ember/jquery in an addon's dependency is not enough, instead we expect the user to explicitly turn the integration flag on to force him to make that call?
But at least we would have to show him a clear warning message if @ember/jquery is present but the integration is not turned on.
Agree. If a non-project depends on @ember/jquery
and jquery-integration: false
is specified, we should warn/error. I like your suggested error/warning message.
So maybe the bundling-only use case should be addresses by a separate (user-land?) addon like ember-jquery? (#1). Or just refer to Embroider/auto-import?
Yeah, I suppose. Do we just tell folks to add an app.import
(or use ember-auto-import) themselves?