-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6349] Bump Angular from 9 to 13 #5109
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
Conversation
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
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.
It silences linting errors of using dot characters(.) in object keys.
| import { OP } from './message-operator.interface'; | ||
|
|
||
| export type MixMessageDataTypeMap = MessageSendDataTypeMap & MessageReceiveDataTypeMap; | ||
| export type MessageDataTypeMap = MessageSendDataTypeMap | MessageReceiveDataTypeMap; |
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.
- After upgrading to TS4, using an intersection of send/receive maps caused overlapping
OPkeys (e.g.LIST_NOTE_JOBS) to intersect their payload types (e.g.undefined & ListNoteJobs), producingnever. - This broke
WebSocketMessage<keyof MessageSendDataTypeMap>becauseMixMessageTypeMap[K]evaluated toneverfor someK, leading to generic constraint errors (TS2344) and lost inference. - Switching
MixMessageTypeMapto a union(Send | Receive)restores correct indexed access(Send[K] | Receive[K])and fixes the errors. - Also updated
WebSocketMessageto a discriminated-union friendly form for stable inference. (Type-only change; no runtime impact.)
Note
To learn how applying keyof operator to union type or intersection type works, see https://effectivetypescript.com/2024/08/30/keyof-puzzle/
TL;DR
keyof (A & B)=(keyof A) | (keyof B)keyof (A | B)=(keyof A) & (keyof B)
| (startJob)="onStartJob($event)" | ||
| (stopJob)="onStopJob($event)" |
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.
Fixed name conflicts with native events(start, stop)
zeppelin-web-angular/package.json
Outdated
| "build-project:sdk": "ng build --project zeppelin-sdk", | ||
| "build-project:vis": "ng build --project zeppelin-visualization", | ||
| "lint": "cross-env NODE_OPTIONS='--max-old-space-size=8192' ng lint && tslint -p tslint-rules/tsconfig.json && prettier --check \"**/*.{ts,js,json,css,html}\"", | ||
| "lint:fix": "cross-env NODE_OPTIONS='--max-old-space-size=8192' ng lint --fix && tslint --fix -p tslint-rules/tsconfig.json && prettier --write \"**/*.{ts,js,json,css,html}\"", |
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.
Added --max-old-space-size=9128 options to fix heap out of memory error.
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.
My environment shows the above warning when running locally. Could you take a look?
Thank you very much for handling this challenging task. I’ve done a brief review, but I’ll keep an eye on it just in case. I’ll also try cherry-picking the unmerged e2e PR to see if it passes tests. I appreciate your effort.
zeppelin-web-angular/projects/zeppelin-visualization/.eslintrc.json
Outdated
Show resolved
Hide resolved
| { | ||
| "files": ["*.ts"], | ||
| "parserOptions": { | ||
| "project": ["projects/zeppelin-sdk/tsconfig.lib.json", "projects/zeppelin-sdk/tsconfig.spec.json"], |
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.
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 set parserOptions.project to true and unified the per-project config filenames from tsconfig.*.json to tsconfig.json. With this, @typescript-eslint/parser automatically picks the nearest tsconfig.json for each file instead of relying on hard-coded project paths, which avoids the path-resolution error we saw.
If/when we upgrade Angular further and adopt ESLint's flat config, we can switch to a javascript flat config and keep paths relative to the config file location using __dirname, which should remove this class of issues.
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.
https://github.com/apache/zeppelin/actions/runs/18752067872/job/53494105014?pr=5109
It seems like a good direction, but this might be the reason why the e2e tests are failing.
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 added path mapping in e2e/tsconfig.json and it seems to work.
| } from '@angular/core'; | ||
| import { DomSanitizer, SafeHtml, SafeUrl } from '@angular/platform-browser'; | ||
| import { default as AnsiUp } from 'ansi_up'; | ||
| import { AnsiUp } from 'ansi_up'; |
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.
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 can’t reproduce this locally. [email protected] ships its own type definitions. Could you try again on your end? If it still fails, a clean install (remove node_modules and the lockfile, then reinstall) or sharing your TS version and the exact error would help.
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.
It seems that .browserslistrc exists in both zeppelin-web-angular/ and zeppelin-web-angular/src/. Aside from comments, there doesn’t appear to be any difference between them. So I think it should be fine to keep just one of them.
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 removed src/.browserslistrc.
zeppelin-web-angular/package.json
Outdated
| "ts-node": "~7.0.0", | ||
| "tsickle": "0.38.1", | ||
| "tslint": "~5.15.0", | ||
| "tslint-no-circular-imports": "^0.7.0", |
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.
If you're removing TSLint, I think you can also remove nz-tslint-rules and tslint-no-circular-imports.
For any custom rules, you can either add them as ESLint rules if possible, or create an issue for them if not.
After that, you can remove the tslint-rule folder and the postinstall build step (npm run build:tslint-rules), and update the lint and lint:fix scripts to use ESLint instead.
It seems that src/tslint.json should be removed as well.
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’ve removed all TSLint-related code for now. If we need any custom lint rules, we can re-implement them later.
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.
https://issues.apache.org/jira/browse/ZEPPELIN-6372
I’ve opened an issue for this, and I’ll add it later.
|
I've fixed failed |
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.
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 think replacing this with reliable, public APIs won't be straightforward. The Angular team has opposed exposing a runtime API that compiles string templates for security and performance reasons.
(angular/angular-cli#9306 (comment))
We've also had to implement workarounds for related runtime-compiler/AOT issue. (#4809)
Given that, we may consider alternative approaches to rendering dynamic templates with data binding.
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.
, or else we could deprecate %angular interpreter.
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.
Thank you for checking instead. As you mentioned, since replacing it isn’t straightforward, I think it’s fine to leave it as is if there are no issues with its functionality. Deprecating the Angular interpreter for this would mean we won’t be able to upgrade it in the future, so that doesn’t seem like a good approach.
I’ve confirmed that all the reviews I left earlier are functioning correctly. Thank you for the great work, and I’ll take a closer look before approving. Thanks again.
| > | ||
| <span style="vertical-align: middle; display: inline-block; margin-top: 3px; color: #333;">Available Fields</span> | ||
| <div style="clear: both;"></div> | ||
| <span style="vertical-align: middle; display: inline-block; margin-top: 3px; color: #333">Available Fields</span> |
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.
It would be nice to have a consistent rule for this as well, since in some cases the styles are aligned with line breaks and in others they are not.
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 think maybe printWidth option do that work. Also, as we use (almost) the latest prettier now, we won't face major reformatting for a while.
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.
Then I think we can consider this part later. Thanks!
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.
https://github.com/dididy/zeppelin/actions/runs/18786968912
https://github.com/dididy/zeppelin/commits/pr/5109-e2e-2/
I created a new branch(e2e test purpose) based on current PR, merged #5101, #5102, and #5105 into it, and ran the tests — all of them passed. Since the core notebook-related features in Zeppelin are functioning properly, it seems there are no major issues.
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.
LGTM 👍
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…gRootDir" This reverts commit da1b3e3.
This reverts commit 5ca3379.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use relative paths for ng-zorro-antd imports to resolve webpack warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ca994f3 to
0c8e7c2
Compare
### What is this PR for? This PR bumps Angular from 9 to 13 and updates related dependencies in zeppelin-web-angular. It addresses: - https://issues.apache.org/jira/browse/ZEPPELIN-6349 - https://issues.apache.org/jira/browse/ZEPPELIN-6227 (except the custom TSLint rules) I removed the zeppelin-helium and helium-vis-example projects in this PR. The Angular rewrite of Helium was never completed, it’s currently unused, and it introduced circular-dependency-related stack overflows during the application build. Given that, removing these modules is the safer choice. #### Principal version updates - angular: 9 -> 13 - ansi_up: 4 -> 6 - date-fns: 2 -> 3 - tslib: 1 -> 2 - lint-staged: 8 -> 15 - typescript: 3.8 -> 4.6 ### What type of PR is it? Chore ### Todos * [x] - Bump Angular version (Used angular cli) * [x] - Migrate TSLint to ESLint (Used `<at>angular-eslint/schematics:convert-tslint-to-eslint`) * [x] - Fix deprecated code usages * [x] - Bump other minor dependencies * [x] - Remove `zeppelin-helium` and `helium-vis-example` ### What is the Jira issue? [ZEPPELIN-6349](https://issues.apache.org/jira/browse/ZEPPELIN-6349) ### How should this be tested? - Run E2E tests - Manual tests across various pages - Verify `serve` (dev server), `build`, and `lint` work as expected ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #5109 from tbonelee/upgrade-web-angular. Signed-off-by: ChanHo Lee <[email protected]> (cherry picked from commit 358f9e5) Signed-off-by: ChanHo Lee <[email protected]>
|
Thanks, merged into master and branch-0.12 |



What is this PR for?
This PR bumps Angular from 9 to 13 and updates related dependencies in zeppelin-web-angular.
It addresses:
I removed the zeppelin-helium and helium-vis-example projects in this PR. The Angular rewrite of Helium was never completed, it’s currently unused, and it introduced circular-dependency-related stack overflows during the application build. Given that, removing these modules is the safer choice.
Principal version updates
What type of PR is it?
Chore
Todos
@angular-eslint/schematics:convert-tslint-to-eslint)zeppelin-heliumandhelium-vis-exampleWhat is the Jira issue?
ZEPPELIN-6349
How should this be tested?
serve(dev server),build, andlintwork as expectedQuestions: