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

Avoid bundling jquery twice #10498

Merged
merged 6 commits into from Dec 18, 2019
Merged

Avoid bundling jquery twice #10498

merged 6 commits into from Dec 18, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Mar 19, 2019

Closes #10289
Previously: #6864, #6626

cc @arggh

@benjamn benjamn added this to the Package Patches milestone Mar 19, 2019
@benjamn benjamn self-assigned this Mar 19, 2019
@@ -1,7 +1,7 @@
Package.describe({
summary: "Manipulate the DOM using CSS selectors",
// This package currently uses jQuery 1.12.1 (due to #9605).
Copy link
Contributor

@klaussner klaussner Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date now. 🙂

KoenLav
KoenLav approved these changes May 5, 2019
Copy link

@KoenLav KoenLav left a comment

@benjamn is this PR waiting on something specific?

@evolross evolross mentioned this pull request Oct 4, 2019
@afrokick
Copy link
Contributor

@afrokick afrokick commented Nov 8, 2019

As @benjamn said in #10522 (comment)

I would love to see that merged, but the test failures indicate that the major version bump (to jQuery 3) is causing version solver conflicts. This is no surprise, since packages that depend on jquery almost certainly need to be updated to work with jQuery 3 instead of 1.12.1 (or maybe they can stop using jQuery entirely), but I'm not sure how much work that will take.

@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Nov 8, 2019

Ok, so the following packages depend on jquery:
core

  1. tests-in-browser -> Works with jquery 3.4.1. But doesn't seem like it even needs the jquery dependency. This also depends on the deprecated bootstrap package (which has problems with jquery 3), but only for styling. So this bootstrap dependency can easily be removed with some css in the package. No risk invovled.

  2. test-helpers -> Works with jquery 3.4.1, but only uses it for 1 find(), which can be replaced by querySelectorAll these days. No risk involved.

Non-core

  1. Blaze -> would be hard to change, but I verified that it works with version 3.4.1 and doesn't use any deprecated API. All tests pass. However, user applications might be broken due to slight changes in the jquery behavior between 1.12 and 3.4. But that's not much of an issue since this would be marked as a major version upgrade anyway

  2. Spacebars-tests -> Works & passess all tests with jquery 3.4.1. No risk here.

Deprecated

  • Bootstrap -> Bootstrap 1 requires jquery 1. But this package is deprecated. If we break the use of it in tests-in-browsers (easy, since it only uses it for the navbar css), then we can ignore this one.

So should be pretty straightforward.

sebakerckhof added a commit to sebakerckhof/meteor that referenced this issue Nov 12, 2019
Another step towards meteor#10498

The direct jquery dependency was not necessary as far as I can tell. There was a second indirect dependency through the deprecated bootstrap package from which we only need the css.
So I replaced that one with bootstrap 4 css from npm
benjamn added a commit that referenced this issue Nov 15, 2019
This version of Blaze includes meteor/blaze#299 by
@sebakerckhof, which is an important prerequisite for #10498.
sebakerckhof pushed a commit to sebakerckhof/meteor that referenced this issue Nov 25, 2019
This version of Blaze includes meteor/blaze#299 by
@sebakerckhof, which is an important prerequisite for meteor#10498.
benjamn pushed a commit that referenced this issue Nov 26, 2019
…#10773)

* remove jquery and deprecated bootstrap dependency in test-in-browser

Another step towards #10498

The direct jquery dependency was not necessary as far as I can tell. There was a second indirect dependency through the deprecated bootstrap package from which we only need the css.
So I replaced that one with bootstrap 4 css from npm

* Improve styling for test-in-browser
@benjamn benjamn force-pushed the avoid-bundling-jquery-twice branch from c1ff377 to ec767dd Compare Nov 26, 2019
@benjamn benjamn changed the base branch from release-1.8.1 to devel Nov 26, 2019
@benjamn benjamn removed this from the Package Patches milestone Nov 26, 2019
@benjamn benjamn added this to the Release 1.8.3 milestone Nov 26, 2019
@benjamn benjamn changed the base branch from devel to release-1.8.3 Dec 18, 2019
benjamn and others added 6 commits Dec 18, 2019
Although the Meteor jquery package is no long a core package (and thus is
not tied to the Meteor release), it seems like a good idea to nudge folks
towards installing jquery from npm, instead of relying on the very old
version (1.12.1) residing in meteor/packages/non-core/jquery/jquery.js.

Closes #10289.
@benjamn benjamn force-pushed the avoid-bundling-jquery-twice branch from 3cc3b9c to 73d82d7 Compare Dec 18, 2019
@benjamn benjamn merged commit 7e753ff into release-1.8.3 Dec 18, 2019
1 of 6 checks passed
benjamn added a commit that referenced this issue Dec 18, 2019
Although the Meteor jquery package is no long a core package (and thus is
not tied to the Meteor release), it seems like a good idea to nudge folks
towards installing jquery from npm, instead of relying on the very old
version (1.12.1) residing in meteor/packages/non-core/jquery/jquery.js.

Closes #10289.
benjamn added a commit that referenced this issue Dec 18, 2019
Although the Meteor jquery package is no long a core package (and thus is
not tied to the Meteor release), it seems like a good idea to nudge folks
towards installing jquery from npm, instead of relying on the very old
version (1.12.1) residing in meteor/packages/non-core/jquery/jquery.js.

Closes #10289.
@benjamn benjamn removed this from the Release 1.8.4 milestone Dec 18, 2019
@benjamn benjamn added this to the Release 1.8.3 milestone Dec 18, 2019
@benjamn benjamn mentioned this pull request Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants