rusackas commented on issue #39539:
URL: https://github.com/apache/superset/issues/39539#issuecomment-5943101650
Cross-linking #44726 (`feat(build): migrate to Typescript 6`, @hainenber),
since it's an independent attempt at this same migration that isn't part of
this phased chain. Took a look to see whether it should supersede the remaining
phases here.
Short version: it gets a couple of things right that are worth carrying into
the eventual config-cleanup phase, but as written it can't land as-is, since it
does the one thing this whole phased approach exists to avoid.
**Worth keeping an eye on:** it drops the per-package `baseUrl` override in
favor of explicit `paths` resolved relative to each package's own tsconfig,
root included. That matches this issue's stated preference for the
config-cleanup phase ("Migrate the root tsconfig.json off baseUrl... the
migration-off-baseUrl path is preferred") rather than just adding
`ignoreDeprecations`. Worth pulling that specific change into the
config-cleanup PR when we get there.
**The blocker:** the root `tsconfig.json` diff adds `"strict": false` with
the comment "Disable strict mode temporarily for Typescript v6 migration."
That's a global type-safety regression across the entire monorepo, not a TS 6.0
forward-compat fix, and it's the opposite of what the per-phase measurement
here is trying to produce (a real, verifiable reduction in TS 6.0 errors). The
four chart-controls fixes it does include (explicit param types on a few
`.filter`/`.some` callbacks) look reasonable on their own, but they were almost
certainly measured with strict mode off, so there's no way to know whether
they're the real, complete Phase D fix set or just whatever happened to still
surface with strict checking disabled. `lint-frontend` is also currently
failing on it.
Given that, I'd treat #44726 as a useful signal (and a source to borrow the
`paths`-based baseUrl fix from later) rather than something to merge over this
chain. Picking up Phase D now, scoped and measured with strict mode intact,
same as B/C/E.
Also noticed this issue's checklist is stale: Phase A (#39530), Phase B
(#39535), Phase C (#39537), and Phase E (#40028) are all already merged.
Updating the checklist to match.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
