Angular upgrade v17 to v18 #7099
Conversation
arcra
left a comment
arcra
left a comment
There was a problem hiding this comment.
From the PR description:
The patch file was also rewritten for v18 because the new tooling has a code-removal step that deletes parts of the app and leaves the page blank; the patch turns it off.
This existed in the previous version too, right? In the PR description it sounds like a new change. Can we just say that we update the patch for v18?
Updated the patch name in WORKSPACE; removed the old entry from whitespace_hygiene_test.py.
I don't see anything related to whitespace_hygiene_test.py, is this out of date? Or did I miss something?
| data = [ | ||
| "//patches:@angular+build-tooling+0.0.0-2113cd7f66a089ac0208ea84eee672b2529f4f6c.patch", | ||
| "//patches:@angular+build-tooling+0.0.0-db91da4e742cd081bfba01db2edc4e816018419b.patch", | ||
| "//patches:@bazel+concatjs+5.8.1.patch", |
There was a problem hiding this comment.
I remember on the previous upgrade (#7085 ), there was a comment about the upgrade to concatjs 5.8.1.
I was under the impression we'd need to do this for this next migration. Was this not the case? This might be a bit related with upgrading our bazel set-up with new dependencies, potentially using bazel modules, that Pablo might be working on at some point, so perhaps this is the reason?
There was a problem hiding this comment.
Current versions @bazel/concatjs, @bazel/esbuild, @bazel/typescript: still 5.8.1 are still compatible with Angular 18.
I plan to wait until Angular dependencies are not compatible anymore with @bazel(-angular) dependencies to update those packages. If we reach 21 without replacing @bazel/concatjs with rules_nodejs I can create a follow-up task to do this major change.
rules_nodejs 6.x is a major change, it removes @bazel/concatjs, @bazel/esbuild, @bazel/typescript entirely and replaces them with the new rules_js / aspect_rules_js system. It rewrites how Bazel loads npm packages and runs TypeScript and esbuild.
cdavalos7
changed the title
|
Updated PR comment, thank you for pointing out @arcra. Yes, there is a change in the file |
cdavalos7
force-pushed
the
feature/upgrade17-18
branch
from
cf5f986 to
619752c
Compare
| - plugins.push(devkitOptimizePlugins.pureToplevelFunctionsPlugin); | ||
| - plugins.push(markTopLevelPure); | ||
| - } | ||
| + // For TensorBoard: This plugin aggressively culls symbols in a way that |
There was a problem hiding this comment.
It seems like all of this is being commented out from the patch? Should it just be removed? Or am I misunderstanding something?
There was a problem hiding this comment.
Because its a patch from a file that exists in node_modules it is best practice to comment out the original code instead of deleting it, so any developer can know what was deleted from the original file.
I can delete it, but I think is better to keep to know what was disabled from node_modules/@angular/build-tooling/shared-scripts/angular-optimization/esbuild-plugin.mjs
| # This is required by patch format and cannot be removed. | ||
| exceptions = frozenset( | ||
| [ | ||
| "patches/@angular+build-tooling+0.0.0-2113cd7f66a089ac0208ea84eee672b2529f4f6c.patch", |
There was a problem hiding this comment.
I realize this is a test, but does this mean the patch is no longer needed? It seems the file still exists, above. Should that file just be removed altogether? Or is it the case that it's not needed only for the test, for some reason?
There was a problem hiding this comment.
The list of frozenset exceptions it's just the list of files the whitespace test skips. Package patches add trailing spaces on blank lines (the patch format needs them), so those files would fail the check otherwise.
It fails in the CI/lint that essentially reviews formatting of the files.
This patch for this version was wrote by hand without those trailing spaces, so it has no whitespace issues to skip. The test also fails if you list a file that doesn't actually need it, so keeping it there would just break CI. Only the protobuf patch still has a trailing space, so that's the only one left.
I hope my explanation was clear about it, let me know otherwise
| "@types/d3-drag": "1.2.8", | ||
| "@types/d3-scale-chromatic": "1.5.4" | ||
| "@types/d3-scale-chromatic": "1.5.4", | ||
| "selenium-webdriver": "4.27.0" |
There was a problem hiding this comment.
This seems to be a new dependency? Do you know if this was bundled with something else before this version, and that's why it's added here now?
There was a problem hiding this comment.
Yes, it was bundled before under the scenes pulled by other libraries. The new v18 build tooling asks for a newer version of it, and yarn would normally grab the latest one. But the latest versions need Node 20, and we're still on Node 18, so they'd break our build.
We shouldn't need it when we have node.js 20.
2 participants
Motivation for features / changes
This PR is the third step in a planned major Angular upgrade cycle, where each major version will be delivered in a separate PR, incrementally progressing until the project reaches Angular 21. This specific PR upgrades TensorBoard from Angular 17 to Angular 18
Technical description of changes
Detailed steps to verify changes work correctly (as executed by you)