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

build(deps): use jquery v2.2.4 to unblock jquery-ui v1.14.1 upgrade #11109

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Dec 10, 2024

What

The jquery-ui upgrade suggested in #10970 is incompatible with jquery versions earlier than v2.2.0 as it lacks a fix that was applied for jquery/jquery#2466

In our package-lock.json file, it appears that we use jquery v3.7.1 -- but in fact we copy a vendored copy from the foundation UI framework, during the gulpfile.ts copyJs function.

I believe that jquery v2.2.0+ is compatible -- and have tested this locally. Compatibility was restored by jquery/jquery@bf591fb because it ensures that a uniqueSort function (as used by jquery-ui) is available in jquery.

Related issue(s) and discussion

Edit: cleanup pull request description.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Dec 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.03%. Comparing base (a0a975a) to head (84c593e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11109   +/-   ##
=======================================
  Coverage   49.03%   49.03%           
=======================================
  Files          78       78           
  Lines       22374    22374           
  Branches     5363     5363           
=======================================
  Hits        10971    10971           
  Misses      10058    10058           
  Partials     1345     1345           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -104,7 +107,6 @@ function buildjQueryUi() {
"./node_modules/jquery-ui/ui/position.js",
"./node_modules/jquery-ui/ui/keycode.js",
"./node_modules/jquery-ui/ui/unique-id.js",
"./node_modules/jquery-ui/ui/safe-active-element.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: safe-active-element.js was removed from jquery-ui in jquery/jquery-ui@bb49bd7 (released in v1.14.0).

@jayaddison
Copy link
Contributor Author

jayaddison commented Dec 10, 2024

From local testing: jquery v3.7.1 does appear to work OK too. Let's upgrade to that instead.

Edit: subsequent testing uncovered a compatibility problem related to the preferences panel.

@jayaddison jayaddison force-pushed the pr-10970-unblock/upgrade-jquery-v2.2.4 branch from 92053af to 315ca55 Compare December 10, 2024 18:50
@jayaddison jayaddison changed the title build(deps): use jquery v2.2.4 to unblock jquery-ui v1.14.1 upgrade build(deps): use jquery v3.7.1 to unblock jquery-ui v1.14.1 upgrade Dec 10, 2024
@github-actions github-actions bot added the 🏷️ Folksonomy Project The Folksonomy project allows to add custom data to product, and model non food products. label Dec 10, 2024
@jayaddison
Copy link
Contributor Author

Note: I have only manually tested the nutrition autosuggest functionality so far (it's the main affected component I'm aware of and familiar with), but I think this pull request is ready for review.

@jayaddison
Copy link
Contributor Author

I'm encountering a JavaScript error when attempting to close the preferences-edit panel using a build from fd7fb7f.

It seems likely that the foundation code is not forwards-compatible with such a significant jquery version upgrade.

image

@jayaddison jayaddison marked this pull request as draft December 10, 2024 19:46
@jayaddison jayaddison force-pushed the pr-10970-unblock/upgrade-jquery-v2.2.4 branch from fd7fb7f to 935e7e0 Compare December 10, 2024 20:16
@jayaddison jayaddison changed the title build(deps): use jquery v3.7.1 to unblock jquery-ui v1.14.1 upgrade build(deps): use jquery v2.2.4 to unblock jquery-ui v1.14.1 upgrade Dec 10, 2024
@jayaddison jayaddison marked this pull request as ready for review December 10, 2024 21:12
@alexgarel
Copy link
Member

@jayaddison thanks for working on this !

@stephanegigandet
Copy link
Contributor

Thank you so much @jayaddison ! I'll do my best to test the PR locally.

@stephanegigandet
Copy link
Contributor

What's the best way to have the gulpfile.ts changes applied in the dev containers? (ideally without running "make dev")

I tried "make front_npm_update", "make front_build" followed by "make restart" but I still have jquery ui 1.13.3

I also tried docker compose up -d --build frontend
but I'm getting an error:

 > [openfoodfacts-server/frontend:dev builder 13/13] RUN npm run build:
#0 0.409 
#0 0.409 > product-opener@1.0.0 build
#0 0.409 > gulp
#0 0.409 
#0 1.030 [11:36:08] Requiring external module ts-node/register
#0 2.230 TypeError: Unknown file extension ".ts" for /opt/product-opener/gulpfile.ts

@jayaddison
Copy link
Contributor Author

What's the best way to have the gulpfile.ts changes applied in the dev containers? (ideally without running "make dev")

There may be a better way, but so far I have been running make down, make clean and then subsequently SKIP_SAMPLE_IMAGES=1 make dev (I noticed the env var in the Makefile as a way to save some time on each iteration) to bring up a fresh environment. This does also require recreating a user account each time.

@stephanegigandet
Copy link
Contributor

What's the best way to have the gulpfile.ts changes applied in the dev containers? (ideally without running "make dev")

There may be a better way, but so far I have been running make down, make clean and then subsequently SKIP_SAMPLE_IMAGES=1 make dev (I noticed the env var in the Makefile as a way to save some time on each iteration) to bring up a fresh environment. This does also require recreating a user account each time.

Thanks @jayaddison . I did that, but after I'm missing jquery, the file http://static.openfoodfacts.localhost/js/dist/jquery.js does not exist for some reason on my dev environment.

dependabot bot and others added 5 commits December 23, 2024 10:48
Bumps the jquery group with 1 update in the / directory: [jquery-ui](https://github.com/jquery/jquery-ui).


Updates `jquery-ui` from 1.13.3 to 1.14.1
- [Release notes](https://github.com/jquery/jquery-ui/releases)
- [Commits](jquery/jquery-ui@1.13.3...1.14.1)

---
updated-dependencies:
- dependency-name: jquery-ui
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: jquery
...

Signed-off-by: dependabot[bot] <support@github.com>
@jayaddison jayaddison force-pushed the pr-10970-unblock/upgrade-jquery-v2.2.4 branch from d606852 to 84c593e Compare December 23, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file 🏷️ Folksonomy Project The Folksonomy project allows to add custom data to product, and model non food products. JavaScript
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Release v2.45.0 broke the ability to add nutrients and other features are showing issues
6 participants