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?
| # 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?
| "@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?
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)