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
merged 2 commits into from Jan 29, 2019

Conversation

Projects
None yet
2 participants
@rwjblue
Copy link
Member

rwjblue commented Jan 28, 2019

Fixes #263

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jan 28, 2019

@simonihmig - Would you mind sanity checking this one?

@rwjblue rwjblue force-pushed the rwjblue:only-blacklist-jquery-when-parent-depends-on-it branch from 42e6003 to 6a40015 Jan 28, 2019

@simonihmig

This comment has been minimized.

Copy link
Contributor

simonihmig commented Jan 28, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jan 28, 2019

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

@simonihmig

This comment has been minimized.

Copy link
Contributor

simonihmig commented Jan 29, 2019

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 babel:master Jan 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the rwjblue:only-blacklist-jquery-when-parent-depends-on-it branch Jan 29, 2019

@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