MartijnVisser commented on PR #28610:
URL: https://github.com/apache/flink/pull/28610#issuecomment-4934886327
Thanks for the PR @spuru9!
This is a first-round pass on the diff, since the branch needs a rebase on
`master` before it's mergeable and I haven't done the full local run yet. The
overall approach is sound, and the one part that actually touches the shipped
artifact, the build-system migration, is handled cleanly at the config level:
`outputPath: { "base": "web", "browser": "" }` correctly flattens the
`application` builder's default `browser/` subdirectory back to `web/`, so
packaging is unchanged, and the options that don't apply to the new builder
(`vendorChunk`, `namedChunks`, `buildOptimizer`) were removed.
A few things I verified independently so we don't need to re-check them:
- **package-lock.json** reconciles with a devDependencies-only change: the
`@angular-devkit/build-angular` (webpack) subtree comes out, `@angular/build` +
`vitest` + `jsdom` go in, and the `dependencies` block in `package.json` is
byte-for-byte unchanged. No runtime dependency drifted (spot-checked version +
integrity hash for the core `@angular/*` and `tslib` entries).
- **NOTICE** needs no change: all three added deps are devDependencies and
aren't bundled into the production app. Good call regenerating against `npm ci
--omit=dev` to confirm.
- **pom.xml** correctly adds `tsconfig.spec.json` to the RAT exclude list
(JSON can't carry a license header), and the three new `.ts` files all have the
ASF header. The commit message carries the `Generated-by:` trailer as well.
The main thing before I do the full local verification and we merge:
1. Can you confirm you loaded the migrated production build from a running
JobManager and clicked through the views, and not just that `ng build`
succeeds? The `application` builder in its `production` config does its own
critical-CSS inlining and `index.html` script/modulepreload injection under
`--base-href ./`, which is exactly what an exit code won't catch and what the
JM-served dashboard depends on. A quick `npm run build` and then serving the
output behind a real JobManager (Overview / Task Managers / Job Exceptions
rendering + navigation) is enough.
A few smaller things worth fixing while you're in here:
2. `package.json` still lists `@types/jasmine` and `@types/jasminewd2`.
Those are Karma/Jasmine leftovers and unused now that the specs run on Vitest
(`tsconfig.spec.json` sets `"types": ["node"]`), so they can go.
3. `ci-check` now runs `test:ci`, so the specs gate the whole
`flink-runtime-web` Maven build. That's the right call, and the pinned Node
22.16.0 / npm 10.9.0 keeps it deterministic. Just flagging it so we're
conscious the web-UI build is now test-gated on spec stability.
4. The lazy-tab spec in `job-exceptions.component.spec.ts` (clicking the tab
by role/text, then querying `td.clickable`) is the most fragile one, as it
leans on ng-zorro's lazy-tab rendering in jsdom. Testing the `navigateTo(...)`
handler directly, like the following spec already does, would be more robust.
One thing to consider: is the `browser-esbuild` -> `application` builder
migration explicitly in scope for the FLINK-40033 umbrella, or should we note
it on FLINK-40035? It's a genuine prerequisite (the `unit-test` builder needs
the `application` build target), so I'm fine with it living here, I just want
it recorded.
I've assigned FLINK-40035 to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
