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

Incompatibilies with jQuery 3 #13685

Closed
kirmorozov opened this issue Feb 15, 2018 · 29 comments
Closed

Incompatibilies with jQuery 3 #13685

kirmorozov opened this issue Feb 15, 2018 · 29 comments
Assignees
Labels
Fixed in 2.3.x The issue has been fixed in 2.3 release line improvement Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed

Comments

@kirmorozov
Copy link
Member

We'd like to use tools that are standard in web development industry.
This includes latest jQuery.

Preconditions

  1. I use Bootstrap 4
  2. I want to use navigation and other scripts.

Steps to reproduce

  1. npm install jquery
  2. Copy jquery.js to you your theme
  3. Build theme.
  4. Open product page with configurable product.

Expected result

  1. No errors

Actual result

  1. size() is not defined (http://api.jquery.com/size/ was deleted from jQuery 3)
@kirmorozov kirmorozov self-assigned this Feb 15, 2018
@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Feb 15, 2018
kirmorozov added a commit to kirmorozov/magento2 that referenced this issue Feb 16, 2018
@magento-engcom-team magento-engcom-team added the Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed label Feb 16, 2018
@magento-engcom-team
Copy link
Contributor

@kirmorozov, thank you for your report.
We've acknowledged the issue and added to our backlog.

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Feb 16, 2018
@orlangur
Copy link
Contributor

Such bug report is not valid. While jQuery, like any other third party library, can definitely be upgraded via one or more PRs, core is not supposed to be forward compatible with any version but only with ones actually used.

@orlangur orlangur added improvement and removed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Feb 16, 2018
magento-engcom-team added a commit that referenced this issue Feb 21, 2018
… with jQuery 3.* #13686

 - Merge Pull Request #13686 from kirmorozov/magento2:2.3-develop-13685
 - Merged commits:
   1. 52875a8
magento-engcom-team added a commit that referenced this issue Feb 21, 2018
Accepted Public Pull Requests:
 - #13735: [Forwardport] Fix adding values to system variable collection (by @nmalevanec)
 - #13733: [Forwardport] Refactoring: remove unuseful temporary variable (by @nmalevanec)
 - #13731: [Forwardport] Display a more meaningful error message in case of misspelt module name (by @nmalevanec)
 - #13727: [Forwardport] Show maintenance IP-address without commas (by @nmalevanec)
 - #13729: [Forwardport] Update StorageInterface.php (by @nmalevanec)
 - #13635: [Forwardport] #13498 issue #13497 - Method getUrl in Magento\Catalog\Model\Product\Attribute\Frontend\Image (by @nmalevanec)
 - #13686: #13685: Replaced .size() with .length to be compatible with jQuery 3.* (by @kirmorozov)
 - magento-engcom/magento2ce#1203: Report error csv doesn't work when trying to import a csv file with semicolon delimiter[forwardport]. (by @nmalevanec)
 - #13361: Fix URL passed to static.php in PHP in-development server (by @nieltg)


Fixed GitHub Issues:
 - #5015: Report error csv doesn't work when trying to import a csv file with semicolon delimiter (reported by @agoeurysky) has been fixed in magento-engcom/magento2ce#1203 by @nmalevanec in 2.3-develop branch
   Related commits:
     1. 7c03614
@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Feb 22, 2018
@magento-engcom-team
Copy link
Contributor

Hi @kirmorozov. Thank you for your report.
The issue has been fixed in #13688 by @kirmorozov in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.0 release.

magento-engcom-team added a commit that referenced this issue Mar 17, 2018
…ith jQuery 3.x #13688

 - Merge Pull Request #13688 from kirmorozov/magento2:2.3-develop-13685-jquery-mobile
 - Merged commits:
   1. c36fb76
   2. b21f2a6
kirmorozov added a commit to kirmorozov/magento2 that referenced this issue Apr 5, 2018
* Updating jQuery to 3.3.1 (originally from npm)
* Updating jQuery Migrate 3.0.1 (originally from npm)
@kirmorozov
Copy link
Member Author

Main concern was .browser from #14267
Its resolved.

@DanielRuf
Copy link
Contributor

Sure.

Just to mention it

It was just meant as notice.

@DanielRuf
Copy link
Contributor

This upgrade would be very useful if it can be included in the next major release.

Totally agree with this.

@kirmorozov
Copy link
Member Author

@VladimirZaets was assigned to latest PR,
There might be 3-4-5 other issues found after complete MFTF run, can be resolved easily.

@zetlen
Copy link

zetlen commented Jun 21, 2018

If jQuery can be upgraded to the 3.x release stream without significant regression, then we could do it. But we can't set a precedent where we keep all of the core lib/ support libraries up to date. A lot of working software may be reliant on behaviors that are peculiar to those versions. Semantic versioning can only protect us so much from this.

I sympathize with @leoquijano in being forced to use older versions of plugins, but the jQuery install base is still largely 1.x, and so the community of plugin authors usually do maintain 1.x compatibility. Alternatives are likely to exist.

So again, in the individual case, I support experimenting with jQuery 3, but I think it's likely that an upgrade would break more functionality than it would enable.

@leoquijano
Copy link

leoquijano commented Jun 21, 2018

@zetlen, I would be more concerned about what jQuery version is expected by the other libraries, and not by the global use base. If we were concerned by that metric, we wouldn't be using PHP 7 either, since 82.7% of the global installations use v5.x.

Since jQuery is such a critical library, having an old version of it is probably blocking upgrades for the other libraries, and not the other way around. Keep in mind that a jQuery Migrate v3 plugin is available to smooth out the upgrade process. That effectively makes it look like we have v1.12 in terms of backwards compatibility.

@zetlen
Copy link

zetlen commented Jul 4, 2018

@leoquijano We're not officially refusing a jQuery 3 upgrade because of that install base metric. It's just a stat to help us all weigh the costs and benefits.

Again, we're open to a PR to upgrade to jQuery 3, considering its special role as a nearly global dependency. But I want to avoid:

  • Unforeseen regressions in popular third party modules
  • A combination of modules that loads jQuery 3 and jQuery 1.12 at the same time

We all probably want such a PR to have safeguards in place to prevent that last thing in particular. Maybe \Magento\Framework\RequireJs could have a server-side sanity check in place to check for multiple jQuery versions.

@leoquijano
Copy link

Thanks @zetlen. There are a few PRs already in progress, if I understand correctly?

@zetlen
Copy link

zetlen commented Jul 5, 2018

That's right! I'm just stating on this thread our requirements for such a PR to merge. Thanks!

@piotrekkaminski
Copy link
Contributor

@zetlen any update on this one? It is too risky to get into a patch release but it would be super helpful for 2.4 branch as it helps us address major security questions.

@zetlen
Copy link

zetlen commented Aug 15, 2018

@piotrekkaminski Nobody has sent me a jQuery upgrade PR to review, but I also haven't looked for one. The things we are looking for are still the same:

  • an upgrade to current-state jQuery 3
  • "Migration" plugins as necessary to cover breaking changes
  • Refactoring where any regressions occur
  • No visible test regression
  • Some kind of safeguard to prevent a downstream theme from loading more than one jQuery version at once

I would be really excited to see such a change, and though there is a regression risk, I think it's digestible by an ambitious community developer.

@piotrekkaminski
Copy link
Contributor

@zetlen so this PR only solves your first point not the others, right? #15531

@zetlen
Copy link

zetlen commented Aug 15, 2018

@piotrekkaminski It may satisfy the following, too:

  • Refactoring where any regressions occur
  • No visible test regression

But without seeing the output of an end to end test, we can't be sure!

@kirmorozov
Copy link
Member Author

@piotrekkaminski Hello old friend.
This initiative is there since April and I have jQuery 3 in production for front-end.
Its been few months since I put a bit of an effort to fix tests and minor core incompatibilities.
Everything else was pushed to the core before April.

@kirmorozov
Copy link
Member Author

@piotrekkaminski
When do you expect to have end-to-end tests?
I expect to put it in production as early as Magento 2.3 Production release.

kirmorozov added a commit to kirmorozov/magento2 that referenced this issue Nov 28, 2018
Revert "MAGETWO-95280: Storefront Images Don't Scale Back To Original Size In Mobile"

This reverts commit 13d360d
@ghost
Copy link

ghost commented Nov 30, 2018

Hi @kirmorozov, thank you for you report, this issue has already fixed in 2.3-develop branch, and available on 2.3.0 release.

@ghost ghost closed this as completed Nov 30, 2018
@orlangur
Copy link
Contributor

@ghost
Copy link

ghost commented Nov 30, 2018

@orlangur hmm.., but this comment -> #13685 (comment)

kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue Mar 3, 2019
kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue Mar 26, 2019
kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue Mar 26, 2019
kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue Apr 10, 2019
Fixing tabs behaviour with upgraded jquery-ui in admin.
kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue May 30, 2019
kirmorozov added a commit to morozov-group/magento2-edge that referenced this issue May 30, 2019
@Erfans
Copy link
Contributor

Erfans commented Oct 2, 2019

is there any update? since the the jQuery version is still v1.12.4 https://github.com/magento/magento2/blob/2.3.2/lib/web/jquery.js#L2

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.3.x The issue has been fixed in 2.3 release line improvement Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed
Projects
None yet
Development

No branches or pull requests

8 participants