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

Only blacklist jQuery module when parent depends on @ember/jquery itself #264

Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jan 28, 2019

Fixes #263

@rwjblue
Copy link
Member Author

rwjblue commented Jan 28, 2019

@simonihmig - Would you mind sanity checking this one?

@rwjblue rwjblue force-pushed the only-blacklist-jquery-when-parent-depends-on-it branch from 42e6003 to 6a40015 Compare January 28, 2019 15:42
@simonihmig
Copy link
Contributor

So in #263 you described two cases:

  1. any addon has @ember/jquery as a dependency, which would currently make all addons (which have the latest ember-cli-babel) skip the jQuery-import transpilation. But this would not explain the error reported in Dependency on jquery breaks tests in the addon with no jquery ember-test-helpers#545, as at least one addon would have @ember/jquery as a dependency, so the jQuery module shim would be available, right?
  2. if no addon has @ember/jquery as a dependency, but somehow it is found in node_modules, this indeed would explain the error. But I can hardly imagine how that could be the case? Even ember-try would clean things up properly AFAIK?

If we apply the changes here, this could lead to a situation, where a couple of addons might effectively rely on jQuery, but only those that had been updated to list @ember/jquery as a dependency would import jQuery form the shim, and the others would still use ember.$, thus triggering a deprecation, right? Which would work, and maybe even be useful to nudge those addons to list @ember/jquery as a dependency. But on the other hand, if there are unmaintained addons, a user would not be able to quash the deprecation or even update to Ember 4 by just installing @ember/jquery in the app itself, which would be possible right now!

So a bit undecided here 🤷‍♂️
Especially since case 1. above should still work (unless I'm missing something), and 2. is highly unusual.

@rwjblue
Copy link
Member Author

rwjblue commented Jan 28, 2019

But this would not explain the error reported in emberjs/ember-test-helpers#545, as at least one addon would have @ember/jquery as a dependency, so the jQuery module shim would be available, right?

Yes, I'm waiting for a concrete reproduction, but I was able to reproduce the failure by having done npm install @ember/jquery && git checkout package.json (basically installing the addon into node_modules, but not listing it in package.json).

If we apply the changes here, this could lead to a situation, where a couple of addons might effectively rely on jQuery, but only those that had been updated to list @ember/jquery as a dependency would import jQuery form the shim, and the others would still use ember.$, thus triggering a deprecation, right? Which would work, and maybe even be useful to nudge those addons to list @ember/jquery as a dependency.

Yes, exactly.

But on the other hand, if there are unmaintained addons, a user would not be able to quash the deprecation or even update to Ember 4 by just installing @ember/jquery in the app itself, which would be possible right now!

I agree, this seems non-ideal. 🤔 I think we should address this by also checking project deps, what do you think?

@rwjblue
Copy link
Member Author

rwjblue commented Jan 28, 2019

@simonihmig I just pushed another commit with that change, what do you think?

@simonihmig
Copy link
Contributor

I think we should address this by also checking project deps, what do you think?

Yes, I think that makes sense! 👍

@rwjblue rwjblue merged commit ea9b9ab into emberjs:master Jan 29, 2019
@rwjblue rwjblue deleted the only-blacklist-jquery-when-parent-depends-on-it branch January 29, 2019 14:02
@rwjblue rwjblue added the bug label Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants