-
Notifications
You must be signed in to change notification settings - Fork 69
feat: Update Node toolchain to version 24 for crud-web-apps #749
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
base: notebooks-v1
Are you sure you want to change the base?
feat: Update Node toolchain to version 24 for crud-web-apps #749
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4ae847c to
75da3d4
Compare
|
Thank you, Yehudit |
andyatmiami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yehudit1987 - great start here! Wanna get these first round of review changes incorporated to kinda "lock things in" - and then I will focus on testing/live-reviewing a running cluster with these changes.
One other area I'm truly not sure on - but commenting here for transparency - is what to do about the @types/node dependency in related package.json files 🤔
"@types/node": "^12.11.1",is what all web-apps declare indevDependenciescurrently- this was already incorrect as ideally we should have been using ``"@types/node": "^16.y.z"
forbullseye` NodeJS release 😢 - further unclear then what we wanna do in this PR?
- update to
^24.y.z - update it to
^20.y.z- still wrong - but at least a little more current
- leave it alone
- YOLO
- update to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you perform any analysis to understand the (significant) changes being introduced in this file? Note that I have not (yet) done analysis myself - but just seeing the raw numbers at this scale is slightly concerning 😇
3,449 additions13,996 deletions
Deletions are a good thing (normally) - I just wanna ensure we understand and are comfortable with these edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit it's not my expertise here so I will share what I actually did:
1. Ran npm install with Node 24 to regenerate the lockfile
2. Verified the build still works: npm run build
3. Deployed to kind cluster and tested - everything works
4. Checked that package.json has ZERO dependency changes (only added engines field)
5. Confirmed package count is nearly identical (~1,350 packages before/after)
Now I do understand that npm 11 (Node 24) uses a different lockfile format and that massive deletions are mainly format changes. But I'm really not sure what can be counted as a proper analysis beyond that but will be happy to get some recommendations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment on why these import statements needed to change? Not against the change - only that I don't see any reference in the commit message about why this was required... and naively I wouldn't understand why a NodeJS upgrade would require these edits to test files
- particularly since it appears ONLY the
jupyterweb app required this type of adjustment
Furthermore - I'm confused on the stylistic pattern being introduced here:
import { test, expect } from '@playwright/test';
import type { Page } from '@playwright/test';
- here we split the
Page (type)import into its own line
import { setupCustomPage, type CustomPage } from '../support/e2e';
- here we keep the single
importbut prefixCustomPagewithtype
I would prefer to keep the single-line variant if possible 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch on the inconsistency! Let me clarify:
In the most simple words - tests were failing. Reasons:
- JSON import syntax: Node 24 requires
with { type: 'json' }instead ofassert { type: 'json' }(ES2025 spec change). This was a breaking change. - Why only Jupyter: Only Jupyter uses Playwright for E2E tests. Volumes and Tensorboards use Cypress, which doesn't have these import patterns.
I'll update the test files to use consistent inline type prefixes throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on components/crud-web-apps/jupyter/frontend/tests/e2e/form-page.spec.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on components/crud-web-apps/jupyter/frontend/tests/e2e/form-page.spec.ts
.. and also - we now see a 3rd variable on import declarations... whereby type is specified outside the {...} block..
- I assume this would have the effect of enforcing the type qualifier on ALL referenced items in the {...} - which makes sense if my interpretation is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on components/crud-web-apps/jupyter/frontend/tests/e2e/form-page.spec.ts
In the name of consistency - i'm wondering if this should be:
import type { Page } from '@playwright/test';
ORimport { type Page } from '@playwright/test';
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure but my guess would be, have the type inside the brackets only if you have mixed types. So for this case even if it's just one import outside seems better to me.
|
|
||
| # --- Build the frontend --- | ||
| FROM node:16.20.2-bullseye as frontend | ||
| FROM node:24-bookworm as frontend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more deterministic builds - I think we should fix this new version to a major/minor/patch version just like it was originally.
| FROM node:24-bookworm as frontend | |
| FROM node:24.11.1-bookworm as frontend-kubeflow-lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more deterministic builds - I think we should fix this new version to a major/minor/patch version just like it was originally.
| FROM node:24-bookworm as frontend-kubeflow-lib | |
| FROM node:24.11.1-bookworm as frontend-kubeflow-lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I've pinned all Node base images to 24.11.1-bookworm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more deterministic builds - I think we should fix this new version to a major/minor/patch version just like it was originally.
| FROM node:24-bookworm as frontend | |
| FROM node:24.11.1-bookworm as frontend-kubeflow-lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more deterministic builds - I think we should fix this new version to a major/minor/patch version just like it was originally.
| FROM node:24-bookworm as frontend-kubeflow-lib | |
| FROM node:24.11.1-bookworm as frontend-kubeflow-lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more deterministic builds - I think we should fix this new version to a major/minor/patch version just like it was originally.
| FROM node:24-bookworm as frontend | |
| FROM node:24.11.1-bookworm as frontend-kubeflow-lib |
Ok did some research and found this -
As the safest option I would go on 16.y.z, what do you think? |
75da3d4 to
7c6bae9
Compare
Signed-off-by: Yehudit Kerido <[email protected]>
7c6bae9 to
730578c
Compare
feat: Update Node toolchain to version 24 for kubeflow-common-lib ( #728 )
The Dockerfiles needed updating because they build the kubeflow-common-lib,
and npm respects the engines field during 'npm ci', requiring Node 24.
feat: Upgrade Node.js to 24 for Jupyter Web App frontend (#729 )
feat: Upgrade Node.js to 24 for Tensorboards Web App frontend (#730 )
feat: Upgrade Node.js to 24 for Volumes Web App frontend (#731 )
Validation
Local Build:
Integration Testing: