Skip to content

Remove jQuery Migrate #25847

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

Closed

Conversation

theCapypara
Copy link
Member

@theCapypara theCapypara commented Nov 29, 2019

Description (*)

Removes jQuery Migrate

Fixed Issues (if relevant)

  1. Fixes remove jQuery Migrate #21406 : remove jQuery Migrate

Manual testing scenarios (*)

None

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Nov 29, 2019

Hi @Parakoopa. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@theCapypara
Copy link
Member Author

Functional tests are failing.
I don't know for sure if this is, because jquery-migrate is actually required somehow, but it seems only tax rule related tests are failing. Can someone confirm that this is because of the jquery-migrate removal?

@krzksz
Copy link
Contributor

krzksz commented Dec 1, 2019

Hey @Parakoopa, thanks for the contribution! Could you please follow the scenario of one of the failing tests and check what's the problem yourself?

Additionally, I'm not sure if we are allowed to remove jQuery Migrate at all. There can be 3rd party modules and extensions that would break without it and I'm not sure if we can introduce such change before Magento 2.4. I'll try to discuss this with other maintainers.

Copy link
Contributor

@krzksz krzksz left a comment

Choose a reason for hiding this comment

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

Please check the tests, internal Magento modules shouldn't rely on jQuery Migrate.

@ghost ghost assigned krzksz Dec 1, 2019
@ihor-sviziev
Copy link
Contributor

@krzksz I believe we can remove it as part of Magento 2.4 release

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, thank you for the review.
ENGCOM-6386 has been created to process this Pull Request
✳️ @torhoehn, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@krzksz
Copy link
Contributor

krzksz commented Dec 4, 2019

This ticket has to wait for 2.4-develop branch to be introduced so we can change the target of the pull request.

@ihor-sviziev ihor-sviziev added the Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. label Aug 20, 2020
@sidolov
Copy link
Contributor

sidolov commented Aug 20, 2020

Hi @ihor-sviziev , @hostep! Does it make sense to leave jQuery migrate for developer mode only? It will be disabled in production mode but may be helpful for developers, what do you think?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Aug 20, 2020 via email

@hostep
Copy link
Contributor

hostep commented Aug 20, 2020

I was going to say the exact same thing as @ihor-sviziev 🙂
I don't know if it makes sense to keep it included for v3 of jquery if that ever happens?
Developers can always install jquery migrate themselves should they see the need for it.

But I'm not a frontend expert, maybe @krzksz or @DanielRuf or @Parakoopa or @kirmorozov or @zetlen or @leoquijano or @jonathanKingston have a much better founded opinion (sorry for the inclusion guys, all of you are involved in discussions around jquery/jquery v3/jquery migrate here in this repo)

@DanielRuf
Copy link
Contributor

Well, WordPress 5.5 did the same (removing jQuery Migrate) and many plugins and themes were broken - they even announced it a while back but many big plugin developers seem to have forgotten this and users had issues which were resolved by installing the Enable jQuery Migrate Helper plugin in this case.

So we should clearly communicate this with the Magento partners and solution providers (modules / plugins) with a clear roadmap and schedule when this will be released. Otherwise this would catch many by surprise.

@leoquijano
Copy link

I agree that any major changes to jQuery dependencies (e. g. jQuery Migrate) should be rolled out carefully. But keep in mind that this has been going on for a long time. My comment about this dependency blocking newer libraries is already two years old, for example.

A lot of developers (and probably solution partners) are still relying on Magento's default frontend for theming. Having more modern tools would be nice, but if they can't have them, at least jQuery 3 (which is 4 years old) would help them move forward.

@DanielRuf
Copy link
Contributor

I agree that any major changes to jQuery dependencies (e. g. jQuery Migrate) should be rolled out carefully. But keep in mind that this has been going on for a long time. My comment about this dependency blocking newer libraries is already two years old, for example.

A lot of developers (and probably solution partners) are still relying on Magento's default frontend for theming. Having more modern tools would be nice, but if they can't have them, at least jQuery 3 (which is 4 years old) would help them move forward.

I totally agree with you. We should just make sure that this is properly communicated and that we cover this in the docs so we can point them at this then (why does my code not work anymore? We've removed jQuery Migrate. Here's some guide / are some tips to resolve the problems). Maybe a warning in the frontend docs at the top. But this is just an idea and my opinion / my two cents.

@hostep
Copy link
Contributor

hostep commented Aug 20, 2020

Would it make sense to add a simple configuration option in Magento where you can choose to include jquery migrate yes or no? That way it can be kept backwards compatible whilst also giving people who know what they are doing the option to remove it if they so wish to.

It might be a bit awkward though to put this in the adminhtml configuration, since it's very developer oriented. Maybe a better idea would be to provide an option for including/excluding it in the view.xml file of a theme?

Just an idea ...

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 20, 2020

Would it make sense to add a simple configuration option in Magento where you can choose to include jquery migrate yes or no? That way it can be kept backwards compatible whilst also giving people who know what they are doing the option to remove it if they so wish to.

That makes sense, yes.

It might be a bit awkward though to put this in the adminhtml configuration, since it's very developer oriented. Maybe a better idea would be to provide an option for including/excluding it in the view.xml file of a theme?

Just an idea ...

That is a good question, I'm not yet sure which place is the best option.

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @Parakoopa,
I just reviewed failing tests - seems like in some cases w/o jquery migrate magento works incorrectly. Could you double check failing tests and update your changes accordingly?
I believe such fixes might be delivered separately than removing jquery migrate

@sidolov sidolov added Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it Priority: P3 May be fixed according to the position in the backlog. labels Sep 2, 2020
@ihor-sviziev
Copy link
Contributor

Hi @Parakoopa,
Will you be able to update your PR?

@theCapypara
Copy link
Member Author

Sorry, I missed the last comment, I'll check the tests when I find the time.

@ihor-sviziev ihor-sviziev added this to the 2.5 milestone Sep 21, 2020
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev
Copy link
Contributor

Hi @Parakoopa,
I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@m2-assistant
Copy link

m2-assistant bot commented Oct 1, 2020

Hi @Parakoopa, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@gwharton
Copy link
Contributor

Just incase this ever gets picked up again and worked on.

The current implementation of jquery-validate uses $.event.handle...... calls.

handler: function (e) {
var args = arguments;
args[0] = $.event.fix(e);
args[0].type = fix;
return $.event.handle.apply(this, args);
}
};
function handler(e) {
e = $.event.fix(e);
e.type = fix;
return $.event.handle.call(this, e);
}

These are undocumented and deprecated.

Jquery.migrate is required for these calls to work under IE11. Whilst IE11 may not be supported anymore, it still does largely work ok (well on my site atleast) so seems a shame to remove jquery-migrate without first addressing the use of $.event.handle in jquery-validate.

Looks like jquery-validate hasn't been updated to any significant degree in a long time!! :(

@kanevbg
Copy link

kanevbg commented Nov 6, 2020

jQuery migrate is included for a reason. The best thing is to update the jQuery to latest version and update all core algorithms accordingly, than jQuery migrate wont be needed. I believe that likely it can't prove enough value for Magento to do it. Time passes and jQuery 3 gets required for many plugins and systems based on jQuery, which makes this bigger problem in time as it's introducing difficulty in customizing the frontend with newer software.

@hostep hostep mentioned this pull request Dec 23, 2020
@jordanvector
Copy link

Could we not swap the

$.event.handle

with

$.event.dispatch

and then remove jquery migrate? I have been testing this change in IE, so far so good. Will update my answer if I run into any issues.

                handler: function (e) {
                    var args = arguments;
                    args[0] = $.event.fix(e);
                    args[0].type = fix;
                    return $.event.dispatch.apply(this, args);
                }
            };
            function handler(e) {
                e = $.event.fix(e);
                e.type = fix;
                return $.event.dispatch.call(this, e);
            }
        });

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Mar 4, 2021

@jordanvector could you create a pr with your suggested changes, it will be much easier to see the test results ?

@jordanvector
Copy link

jordanvector commented Mar 4, 2021

@ihor-sviziev new PR

#32364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Area: Lib/Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: App Component: Theme Event: cd-cologne19 Partner: Tudock GmbH partners-contribution Pull Request is created by Magento Partner Priority: P3 May be fixed according to the position in the backlog. Progress: needs update Release Line: 2.4 Risk: high Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove jQuery Migrate