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

Update jQuery & jQuery UI versions #844

Merged
merged 13 commits into from
Mar 2, 2024
Merged

Update jQuery & jQuery UI versions #844

merged 13 commits into from
Mar 2, 2024

Conversation

M4LuZ
Copy link
Collaborator

@M4LuZ M4LuZ commented Dec 31, 2023

What is this PR doing?

Updates currently embedded versions of jQuery and jQuery UI.
Functionality was validated (to best knowledge) but it can't hurt to have a few others try this also

Which issue(s) this PR fixes:

Fixes #841

Checklist

  • CHANGELOG.md entry
  • Documentation update no change in functionality / dependency

@M4LuZ M4LuZ added security issue Issues with relation to or with impact on the system security javascript Pull requests that update Javascript code labels Dec 31, 2023
@M4LuZ M4LuZ added this to the LanSuite 5.0 RC milestone Dec 31, 2023
Copy link
Collaborator

@andygrunwald andygrunwald left a comment

Choose a reason for hiding this comment

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

Thats a good one.

Do we know in which places we use JQuery? This would help me to test the change.

ext_scripts/jquery-ui.old/jquery-ui.custom.min.js Outdated Show resolved Hide resolved
ext_scripts/jquery-ui.old/smoothness/jquery-ui.custom.css Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/AUTHORS.txt Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/LICENSE.txt Show resolved Hide resolved
ext_scripts/jquery-ui/external/jquery/jquery.js Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/jquery-ui.css Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/jquery-ui.js Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/jquery-ui.structure.css Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/jquery-ui.theme.css Outdated Show resolved Hide resolved
ext_scripts/jquery-ui/package.json Outdated Show resolved Hide resolved
@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Jan 3, 2024

Thats a good one.

Do we know in which places we use JQuery? This would help me to test the change.

I've identified occurrences by searching the code for $( as to my knowledge that is the sequence to invoke JQuery-specific functionality.
Occurrences can be found in #841, but have it copied here for convenience:

Copy link

sonarcloud bot commented Jan 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

9 Security Hotspots
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Jan 3, 2024

nudging @lansuite/lansuite-developers to have a look, as this may have a larger impact on functionality than currently known

@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Feb 11, 2024

@lansuite/lansuite-developers Given the lack of negative/any feedback I guess we're fine to merge here?
@andygrunwald: how should we proceed in regards to file minifation?

Ship the non-minified version
Ship the minified version only
Ship both and include based on debugging switch
build the minified version ourselves with uglify-js (giving us the original, the minified version and the mapping file)

@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Feb 28, 2024

@lansuite/lansuite-developers Given the lack of negative/any feedback I guess we're fine to merge here? @andygrunwald: how should we proceed in regards to file minifation?

Ship the non-minified version
Ship the minified version only
Ship both and include based on debugging switch
build the minified version ourselves with uglify-js (giving us the original, the minified version and the mapping file)

Reminder to all ;)

@andygrunwald
Copy link
Collaborator

@M4LuZ I tested the PR - Looks good!

About which version we do ship:
In my opinion, should only ship the minified version only (applies to CSS as well).
It is very rare that we discover a bug in jQuery - Hence, debugging might be only in our code required. I don't see a point in shipping the non-minified version at all.
For development, we use the documentation of jQuery.

This would mean deleting all non-minified versions.
Additionally: I don't see a need to ship the LICENSE file, as the license link is also available in the header of the minified version.

After this, I am good with merging this.

@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Mar 2, 2024

Unneeded files removed as discussed

Additionally: I don't see a need to ship the LICENSE file, as the license link is also available in the header of the minified version.

As quoted above the license requirements are clear and the license text (not just a refrence) is to be included.

The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.

Given the size (and the apparent lack of need to optimize space usage or amount of files as per #873) this does not really matter anyways, does it?

@M4LuZ M4LuZ requested a review from andygrunwald March 2, 2024 15:00
Copy link
Collaborator

@andygrunwald andygrunwald left a comment

Choose a reason for hiding this comment

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

Looking good.

@andygrunwald
Copy link
Collaborator

@M4LuZ Space is, indeed not an issue. And we should follow the license. I overlooked this part.

About ext_scripts/jquery-ui/external/jquery/jquery.js
I still would delete this file. Primarily to avoid confusion for developers on why we have two versions of jQuery and why the "external" is not used.

If you agree, I will delete ext_scripts/jquery-ui/external/jquery/jquery.js and merge it through.

Copy link

sonarcloud bot commented Mar 2, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@M4LuZ M4LuZ merged commit 8f41271 into master Mar 2, 2024
6 checks passed
@M4LuZ M4LuZ deleted the 841-jquery-update branch March 2, 2024 19:45
@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Mar 2, 2024

deleted & merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code security issue Issues with relation to or with impact on the system security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade jQuery + jQuery UI versions
2 participants