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

front: switch from yarn to npm #9169

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

Yohh
Copy link
Contributor

@Yohh Yohh commented Oct 3, 2024

close #9143

@emersion emersion self-requested a review October 3, 2024 09:33
@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch from 0845e2d to 068566f Compare October 10, 2024 15:20
@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:integration-tests Work on Integration test, by nature related to different services labels Oct 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.84%. Comparing base (c98d385) to head (8bef3dc).
Report is 22 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##              dev    #9169       +/-   ##
===========================================
+ Coverage   38.19%   79.84%   +41.64%     
===========================================
  Files         998     1053       +55     
  Lines       92197   105088    +12891     
  Branches     1192      757      -435     
===========================================
+ Hits        35219    83909    +48690     
+ Misses      56522    21138    -35384     
+ Partials      456       41      -415     
Flag Coverage Δ
editoast 73.35% <ø> (-0.04%) ⬇️
front 89.34% <ø> (+69.18%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 87.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@emersion
Copy link
Member

I had success running https://github.com/imsnif/synp

I think we'd be less likely to run into issues if we can use this tool instead of crossing fingers.

@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch 5 times, most recently from f93eaa3 to 01593dc Compare October 24, 2024 18:38
@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch 2 times, most recently from fd7c55e to 6e85906 Compare November 6, 2024 14:46
@Yohh Yohh requested a review from ElysaSrc November 18, 2024 10:42
@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch 6 times, most recently from a351913 to 4ef8616 Compare November 18, 2024 15:58
@emersion
Copy link
Member

emersion commented Nov 19, 2024

  • Running npm install before renaming resolutions to overrides in package.json makes npm use @rtk-query/oazapfts-patched instead of our fork.
  • Running npm install after renaming resolutions to overrides in package.json makes npm die with ".git can't be found" in husky install.
  • Trying to upgrade oazapfts to the latest version won't work because it uses a monorepo and Git repo dependencies can't target a subdir.

@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch 2 times, most recently from 32d76db to cb4fea5 Compare November 20, 2024 14:33
@Yohh Yohh marked this pull request as ready for review November 20, 2024 14:33
@Yohh Yohh requested review from a team as code owners November 20, 2024 14:33
@Yohh Yohh requested a review from shenriotpro November 20, 2024 14:33
Copy link
Contributor

@shenriotpro shenriotpro left a comment

Choose a reason for hiding this comment

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

Thanks for the README update

@Khoyo
Copy link
Contributor

Khoyo commented Nov 21, 2024

[khoyo@odin osrd-hotfix]$ git grep yarn
front/README.md:- Install playwright dependencies `cd ./front/ && yarn playwright install --with-deps`
front/README.md:this script as you would to `yarn e2e-tests` or `yarn playwright test`.
front/docker/Dockerfile.playwright:COPY front/package.json front/yarn.lock /app/front/
front/docker/Dockerfile.playwright:RUN yarn install --frozen-lockfile
front/package-lock.json:        "yarn": "^1.7.0"
front/package-lock.json:        "yarn": ">=1"
front/package-lock.json:        "yarn": ">=1"
front/package-lock.json:        "yarn": "*"
front/package-lock.json:    "node_modules/yarn": {
front/package-lock.json:      "resolved": "https://registry.npmjs.org/yarn/-/yarn-1.22.22.tgz",
front/package-lock.json:        "yarn": "bin/yarn.js",
front/package-lock.json:        "yarnpkg": "bin/yarn.js"
scripts/run-front-playwright-container.sh:VERSION=$(yarn list --pattern playwright --json | jq -r '.data.trees[].name | split("@")[-1]' | sort -u)
scripts/run-front-playwright-container.sh:    osrd-playwright:latest yarn playwright test "${args[@]}"

The dependency in the lockfile is probably valid, the rest probably isn't.

@Yohh
Copy link
Contributor Author

Yohh commented Nov 21, 2024

[khoyo@odin osrd-hotfix]$ git grep yarn
front/README.md:- Install playwright dependencies `cd ./front/ && yarn playwright install --with-deps`
front/README.md:this script as you would to `yarn e2e-tests` or `yarn playwright test`.
front/docker/Dockerfile.playwright:COPY front/package.json front/yarn.lock /app/front/
front/docker/Dockerfile.playwright:RUN yarn install --frozen-lockfile
front/package-lock.json:        "yarn": "^1.7.0"
front/package-lock.json:        "yarn": ">=1"
front/package-lock.json:        "yarn": ">=1"
front/package-lock.json:        "yarn": "*"
front/package-lock.json:    "node_modules/yarn": {
front/package-lock.json:      "resolved": "https://registry.npmjs.org/yarn/-/yarn-1.22.22.tgz",
front/package-lock.json:        "yarn": "bin/yarn.js",
front/package-lock.json:        "yarnpkg": "bin/yarn.js"
scripts/run-front-playwright-container.sh:VERSION=$(yarn list --pattern playwright --json | jq -r '.data.trees[].name | split("@")[-1]' | sort -u)
scripts/run-front-playwright-container.sh:    osrd-playwright:latest yarn playwright test "${args[@]}"

The dependency in the lockfile is probably valid, the rest probably isn't.

you're right, I removed the useless mentions in package-lock.json and updated the playwright script, thank you

@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch from d622b9c to 487659a Compare November 21, 2024 12:00
@emersion
Copy link
Member

The package-lock.json references should be retained. In general this file shouldn't be edited by hand. It has "yarn" references because some of our dependencies depend on yarn.

@Yohh Yohh force-pushed the yoh/front-switch-from-yarn-to-npm branch from 487659a to ee40939 Compare November 21, 2024 12:07
@Yohh Yohh requested a review from emersion November 21, 2024 12:11
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

The changes look good to me!

My only remaining worry is that during the various iterations of this PR, the package-lock.json file has evolved and contains unwanted changes. For instance, at some point some dependencies were upgraded in package.json. To ensure we keep the NPM dependency tree as similar as possible to yarn's (and resolve the yarn.lock conflicts with the dev branch), I'd suggest we re-generate package-lock.json from yarn.lock by running npm install on the dev branch and then using the resulting package-lock.json file as-is in this branch. Wdyt?

@emersion emersion force-pushed the yoh/front-switch-from-yarn-to-npm branch from d443e01 to 653bebe Compare November 26, 2024 11:21
@github-actions github-actions bot added area:ci Work on Continous Integration Pipeline Service area:osrdyne labels Nov 26, 2024
@emersion emersion force-pushed the yoh/front-switch-from-yarn-to-npm branch 3 times, most recently from bc73971 to c504a08 Compare November 26, 2024 12:03
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

I've applied my suggestion above, rebased, and also had to add --package-lock-only for npm list to make it work without a prior npm install.

This LGTM, but it would be best if another maintainer could review as well!

@emersion emersion requested a review from Math-R November 26, 2024 12:23
Copy link
Member

@ElysaSrc ElysaSrc left a comment

Choose a reason for hiding this comment

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

Not tested but LGTM

@emersion emersion force-pushed the yoh/front-switch-from-yarn-to-npm branch 3 times, most recently from 38c8eaf to 73130ea Compare December 3, 2024 10:56
Signed-off-by: Yohh <durandyohan@zaclys.net>
Signed-off-by: Simon Ser <contact@emersion.fr>
@emersion emersion force-pushed the yoh/front-switch-from-yarn-to-npm branch from 73130ea to 8bef3dc Compare December 3, 2024 11:00
@emersion
Copy link
Member

emersion commented Dec 3, 2024

Some platform-specific optional dependencies were missing from package-lock.json due to npm/cli#4828, causing breakage on platforms other than Linux amd64. To fix it, dependencies with platform-specific sub-packages need to be re-installed to force npm to not look at node_modules/. I did the following:

npm install --save-dev rollup@4.23.0 @swc/core@1.7.26
git restore package.json
npm install

@Yohh
Copy link
Contributor Author

Yohh commented Dec 4, 2024

everything works fine for me on macos and linux

Copy link
Contributor

@Math-R Math-R left a comment

Choose a reason for hiding this comment

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

Lgtm and tested !

Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@Yohh Yohh added this pull request to the merge queue Dec 4, 2024
Merged via the queue into dev with commit 52ef1ae Dec 4, 2024
27 checks passed
@Yohh Yohh deleted the yoh/front-switch-from-yarn-to-npm branch December 4, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ci Work on Continous Integration Pipeline Service area:front Work on Standard OSRD Interface modules area:integration-tests Work on Integration test, by nature related to different services area:osrdyne
Projects
None yet
Development

Successfully merging this pull request may close these issues.

front: switch from yarn to npm
8 participants