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

Upgrade jQuery to 3.5.x #4929

Merged
merged 5 commits into from Nov 9, 2020
Merged

Conversation

fqueiruga
Copy link
Contributor

@fqueiruga fqueiruga commented Sep 10, 2020

This PR bumps the jQuery version from 2.1.4 to 3.5.x.
I have created the PR in order to properly test it with a CI build.

JS changes:

  • Remove uses of the .size() and .load() methods
  • Remove the unit tests on the maven test command because they have not been ported

Testing command:

  • docker run --rm -it -p 8080:8080 -e ID=4929 jenkins/core-pr-tester

Proposed changelog entries

  • TBD

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@uhafner
@Wadeck
@timja

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

- Remove uses of the .size() and .load() methods
- Remove the unit tests on the maven test command because they have not been ported
@fqueiruga fqueiruga marked this pull request as draft September 10, 2020 08:57
@fqueiruga fqueiruga changed the title [WIP] upgrade jQuery to 3.5.x Upgrade jQuery to 3.5.x Oct 5, 2020
@fqueiruga fqueiruga marked this pull request as ready for review October 5, 2020 14:45
Copy link
Contributor Author

@fqueiruga fqueiruga left a comment

Choose a reason for hiding this comment

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

Taking this one out of WIP status as the proper work is done but there are some discussions and some work left.

Most of the JS unit tests do not work with this PR. These tests are very difficult to migrate and offer little value, as most of the use-cases they cover are also covered by JTH and ATH suites. Any thoughts on this?

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

are you running ATH over this? (more than just the default smoke tests that run on CI)

Most of the JS unit tests do not work with this PR. These tests are very difficult to migrate and offer little value, as most of the use-cases they cover are also covered by JTH and ATH suites. Any thoughts on this?

Can probably remove.

@fqueiruga
Copy link
Contributor Author

I'll get back to you with a full ATH run hopefully tomorrow

@fqueiruga
Copy link
Contributor Author

fqueiruga commented Oct 6, 2020

I have verified that there are no ATH errors that don't happen in other PRs.

@timja timja requested a review from uhafner October 6, 2020 09:28
@timja timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Oct 6, 2020
Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Random manual testing (like monkey typing on keyboards), looking for console errors. Core (without plugins) was working fine, testing with multiple pages.

@timja timja requested a review from romenrg October 6, 2020 10:02
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Nice! This PR makes my jQuery plugin obsolete (if this PR is part of an LTS). Is jQuery 3.x then available as $ in views or do we still need workarounds? (How does this change relate to https://github.com/jenkinsci/jquery-plugin? Is this plugin then obsolete as well?)

Copy link
Member

@romenrg romenrg left a comment

Choose a reason for hiding this comment

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

Just ran some manual smoke tests, clicking random stuff and checking both behavior and the inspect console, as Wadeck did; and everything looks good to me.

@fqueiruga
Copy link
Contributor Author

fqueiruga commented Oct 6, 2020

Nice! This PR makes my jQuery plugin obsolete (if this PR is part of an LTS). Is jQuery 3.x then available as $ in views or do we still need workarounds? (How does this change relate to https://github.com/jenkinsci/jquery-plugin? Is this plugin then obsolete as well?)

It is not. Plugins are still needed for their intended use case. The jQuery version in core is contained to the bundles generated by webpack and not exposed to the window namespace. It should have no effect in any plugin (except those that hook up to the setup wizard).

@fqueiruga
Copy link
Contributor Author

For context, $ in vanilla jenkins (no plugins) corresponds to the prototype object, not jquery

@fqueiruga
Copy link
Contributor Author

I have run a comprehensive PCT suite and have found some errors that I need to go through. I suspect is due to outdated POMs but still, I'd like to have the PR marked as on hold maybe after the LTS selection.

@timja timja added on-hold This pull request depends on another event/release, and it cannot be merged right now squash-merge-me Unclean or useless commit history, should be merged only with squash-merge labels Oct 6, 2020
@timja
Copy link
Member

timja commented Oct 23, 2020

I have run a comprehensive PCT suite and have found some errors that I need to go through. I suspect is due to outdated POMs but still, I'd like to have the PR marked as on hold maybe after the LTS selection.

@fqueiruga should unhold be removed now?

@fqueiruga
Copy link
Contributor Author

TBH I would like to give it a few weeklies of margin, maybe get it in 2.266 or 2.267. The reason is that I don't want the bug reports arising from tables-to-divs to be muddied by this PR, if that makes sense.

@timja
Copy link
Member

timja commented Nov 8, 2020

thoughts on shipping in 2.266?

@MarkEWaite
Copy link
Contributor

thoughts on shipping in 2.266?

I've mentioned that this change is coming in a proposed blog post inviting users and administrators to assist with testing Jenkins weekly releases. I think that including it in Jenkins 2.266 does not complicate bug triage any more than it is already.

@fqueiruga
Copy link
Contributor Author

Let's go then!

@timja timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed on-hold This pull request depends on another event/release, and it cannot be merged right now labels Nov 8, 2020
@timja
Copy link
Member

timja commented Nov 8, 2020

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

It is a bit YOLO, but we should merge it sooner or later. I vote for merging sooner, we have only 1 month before the Christmas break so extra week is a good addition for user feedback

@timja timja merged commit 98ab202 into jenkinsci:master Nov 9, 2020
@liuweiGL
Copy link

@fqueiruga
Copy link
Contributor Author

@liuweiGL can you comment this on the issue? From what I see, your plugin is no loading jQuery in package.json, or in POM.xml. This change was also scoped to just Jenkinsci and should have no sideffects on other plugins.

From what I see, that $ variable may be referring to the prototypejs instance that Jenkins provides. jQuery does not provide a .up() method, but prototype does http://api.prototypejs.org/dom/Element/up/

@fqueiruga
Copy link
Contributor Author

@liuweiGL I think your plugin may have broken on the tables-to-divs release on Jenkins 2.264, given that the change is to DOM transversal on a form function. You could report the issue on the appropiate epic https://issues.jenkins.io/browse/JENKINS-62437 and tag the issue with the label tables-to-divs-regression. More information on how to fix the plugin on https://www.jenkins.io/blog/2020/11/10/major-changes-in-weekly-releases/#plugin-developers

@liuweiGL
Copy link

Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
9 participants