emberjs/ember-jquery

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
    • if `jquery-integration is enabled
      • Ember.$ shows a deprecation for Ember.$
      • import $ from 'jquery'; shows a deprecation for Ember.$
      • this.$() shows a deprecation for this.$()
    • if `jquery-integration is disabled
      • Ember.$ throws an error
      • import $ from 'jquery'; throws an error
      • this.$() throws an error
  • if @ember/jquery is used
    • if `jquery-integration is enabled
      • Ember.$ ??? (not tested)
      • import $ from 'jquery'; works without deprecation warning
      • this.$() shows a deprecation for this.$()
    • if `jquery-integration is disabled
      • Ember.$ throws an error
      • import $ from 'jquery'; works without deprecation warning
      • this.$() throws an error

/cc @rwjblue @chancancode @simonihmig

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 enabled
      • Ember.$ shows a deprecation for Ember.$ - which should be the case already
      • import $ from 'jquery'; works without deprecation warning
      • this.$() should work and show no deprecations
    • if jquery-integration is disabled
      should be fine as described above

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:

  1. 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?

  1. 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?