feat(fe): upgrade `superset-frontend` to Typescript v5 by hainenber · Pull Request #31979 · apache/superset

hainenber · 2025-01-24T09:23:48Z

feat(fe): upgrade superset-frontend to Typescript v5

SUMMARY

Refactor the whole superset-frontend codebase to be compatible with Typescript v5. Most changes are simply casting index key to proper signature, i.e. a as keyof typeof B and defining Record types. However, there are a few hard roadblocks that I had to rely on //@ts-ignore to bypass; I have added explanation for each of them.

Sanity testing from 1st glance looks OK

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

Signed-off-by: hainenber <dotronghai96@gmail.com>

korbit-ai · 2025-01-24T09:23:52Z

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

Signed-off-by: hainenber <dotronghai96@gmail.com>

…alue assignment Signed-off-by: hainenber <dotronghai96@gmail.com>

Signed-off-by: hainenber <dotronghai96@gmail.com>

hainenber · 2025-01-28T13:59:29Z

/korbit-review

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Incorrect Type Return with Optional ID ▹ view	✅
	Zero Value Edge Case in Bubble Size Normalization ▹ view	✅
	Incorrect Type Union in Native Filters Reduction ▹ view
	Incorrect Type Assertion in QueryEditor Accumulator ▹ view	✅
	Overly Restrictive Field Type Casting for className ▹ view
	Incorrect Field Type Casting for helpText ▹ view
	Unsafe numeric conversion in comparator ▹ view	✅
	Lost Hover State Functionality ▹ view	✅
	Potentially Breaking Optional Values Property ▹ view	✅
	Invalid tagIds Type Union ▹ view	✅

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ support@korbit.ai

korbit-ai · 2025-01-28T14:10:28Z

-    helpText={FIELD_TEXT_MAP[field].helpText}
-    label={FIELD_TEXT_MAP[field].label || field}
+    placeholder={FIELD_TEXT_MAP[field as FieldTextMapKey].placeholder}
+    helpText={FIELD_TEXT_MAP[field as 'account']?.helpText}


Incorrect Field Type Casting for helpText

Tell me more

What is the issue?

The field type is incorrectly cast to only 'account', which means helpText will only be available for account fields even when it might be needed for other field types.

Why this matters

This will cause helpText to be undefined for all fields except 'account', potentially hiding important help information from users for other field types.

Suggested change ∙ Feature Preview

Change the type casting to use FieldTextMapKey like other properties:

helpText={FIELD_TEXT_MAP[field as FieldTextMapKey]?.helpText}

_{💡 Does this comment miss the mark? Tell us why and Korbit will adapt to your team’s feedback.

💬 Chat with Korbit by mentioning @korbit-ai.}

@korbit-ai Go with your change doesn't make sense. It returns this issue

Property 'helpText' does not exist on type '{ label: string; helpText: string; placeholder: string; } | { label: string; placeholder: string; className: string; } | { label: string; placeholder: string; className: string; }'. Property 'helpText' does not exist on type '{ label: string; placeholder: string; className: string; }'.

Thanks for your feedback. It seems like 'helpText' does not exist for all the field types in the FIELD_TEXT_MAP. If 'helpText' is not intended for the fields 'warehouse' and 'role', then the current implementation is ok because it will not cause 'helpText' to become undefined. However, if it should exist for each field type, then we may need to adjust the FIELD_TEXT_MAP to include 'helpText' for every field.

hainenber · 2025-01-28T15:51:44Z

Sign in to view

    tagIds = [],
    types,
-  }: { tagIds: number[] | undefined; types: string | null },
+  }: { tagIds: (number | undefined)[] | string; types: string | null },


@korbit-ai The original input would be [tagId?.id] | '' and this will always yield [undefined] or [number]

Hi @hainenber, thanks for your response. I see where you're coming from and appreciate your reasoning. However, to help ensure that our code is clean and minimizes potential confusion, could we perhaps consider using explicit types based on the actual API requirements? For example refactoring our tagIds parameter to be either all numbers or all strings. This would help eliminate any risk of the calling function passing in an invalid value.

@hainenber Do we actually receive an array that might contain undefined values?

korbit-ai · 2025-01-28T14:10:29Z

+    const nativeFilters = Object.keys(filters).reduce(
+      (acc, key) => {
+        acc[key] = pick(filters[key], ['chartsInScope']);
+        return acc;
+      },
+      {} as Record<string, Pick<Filter | Divider, 'chartsInScope'>>,
+    );


Incorrect Type Union in Native Filters Reduction

Tell me more

What is the issue?

The type assertion allows 'Divider' type which cannot have 'chartsInScope' property, potentially causing runtime errors.

Why this matters

If a Divider type is encountered during runtime, accessing 'chartsInScope' will fail as this property doesn't exist on Divider objects, leading to potential undefined behavior or crashes.

Suggested change ∙ Feature Preview

Remove the Divider type from the type assertion as it's not applicable for this operation:

const nativeFilters = Object.keys(filters).reduce( (acc, key) => { acc[key] = pick(filters[key], ['chartsInScope']); return acc; }, {} as Record<string, Pick<Filter, 'chartsInScope'>>, );

_{💡 Does this comment miss the mark? Tell us why and Korbit will adapt to your team’s feedback.

💬 Chat with Korbit by mentioning @korbit-ai.}

@korbit-ai 72th line has this type for the assigned value: Pick<Filter | Divider, "chartsInScope">. Removing Filter doesn't really make sense for the inferred type.

Hi @hainenber, thank you for your reply. You're correct that the assigned value has the type Pick<Filter | Divider, 'chartsInScope'>. I see the potential for misinterpretation, as the Filter objects surely have the 'chartsInScope' field but the Divider objects do not. For the sake of clarity and to prevent any runtime issues, would it be possible to add some error handling or validation to ensure that the Divider type is not being used in a context where 'chartsInScope' is accessed? This would make the code more resilient to errors caused by type mismatches.

…s.ts from Michael's suggestion Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

hainenber · 2025-01-28T14:51:34Z

Can you bump @typescript-eslint/typescript-estree given that newer versions support Typescript 5.7.3?

I tried out npm upgrade @typescript-eslint/typescript-estree but looks like we'll need to upgrade 3 more major versions, from v5 to v8 to get the latest Typescript support.

I'll see if it's feasible within this PR.

michael-s-molina · 2025-01-28T14:56:55Z

I'll see if it's feasible within this PR.

If this is not feasible, an alternative would be to use a Typescript version <5.2.0 such as 5.1.6.

hainenber · 2025-01-28T15:14:09Z

I tried a quick attempt at @typescript-eslint v6 upgrade but the ESLint config doesn't work at all so yeah, 5.1.6 it is :D

Beside, the PR is pretty full so the less mental burden for reviewers, the better :D

…ted Typescript version Signed-off-by: hainenber <dotronghai96@gmail.com>

Signed-off-by: hainenber <dotronghai96@gmail.com>

hainenber · 2025-01-28T16:33:36Z

There seems to be a hanging E2E test process at the moment :D

Signed-off-by: hainenber <dotronghai96@gmail.com>

michael-s-molina · 2025-01-28T20:03:04Z

    tagIds = [],
    types,
-  }: { tagIds: number[] | undefined; types: string | null },
+  }: { tagIds: (number | undefined)[] | string; types: string | null },


@hainenber Do we actually receive an array that might contain undefined values?

Signed-off-by: hainenber <dotronghai96@gmail.com>

…Ids` function Signed-off-by: hainenber <dotronghai96@gmail.com>

hainenber · 2025-01-29T05:15:44Z

@hainenber Do we actually receive an array that might contain undefined values?

Yes, previously in the codebase, there is this reference. I've changed it so as there will be empty string in place of undefined Tag object :D

michael-s-molina

Amazing work @hainenber! Thanks for improving the codebase and addressing the comments.

kgabryje · 2025-01-29T12:04:11Z

I think this PR broke filter indicators.

Before:

After:

michael-s-molina · 2025-01-29T12:06:35Z

@hainenber @kgabryje Can we try to fix it forward?

kgabryje · 2025-01-29T12:07:43Z

+        chart?.queriesResponse?.rejected_filters !==
+          prevChart?.queriesResponse?.rejected_filters ||
+        chart?.queriesResponse?.applied_filters !==
+          prevChart?.queriesResponse?.applied_filters ||


Confirmed that the filter's badge disappeared due to this change - queriesResponse is an array so we need that [0]. Fix in progress

hainenber · 2025-01-29T12:11:17Z

Yikes, this one slipped off my radar. Thanks @kgabryje for doing the fix forward!

kgabryje · 2025-01-29T12:14:37Z

No problem @hainenber!
Fix is here #32025

Signed-off-by: hainenber <dotronghai96@gmail.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

hainenber added 4 commits January 24, 2025 13:41

chore: upgrade Typescript to v5 in dep files

7f1e558

Signed-off-by: hainenber <dotronghai96@gmail.com>

refactor: cast index signature to proper type for a

b30e575

Signed-off-by: hainenber <dotronghai96@gmail.com>

refactor: use type guard for CustomControlItem type

bc15c07

Signed-off-by: hainenber <dotronghai96@gmail.com>

chore: delete unused dashboard util function

1ab5c18

Signed-off-by: hainenber <dotronghai96@gmail.com>

pull-request-size Bot added the size/L label Jan 24, 2025

github-actions Bot added dependencies:npm packages labels Jan 24, 2025

michael-s-molina added the review:draft label Jan 24, 2025

hainenber added 2 commits January 28, 2025 14:26

Merge branch 'master' into feat/upgrade-fe-to-typescript-v5

819758b

feat(fe): refactor codebase for Typescript v5 migration

5305991

Signed-off-by: hainenber <dotronghai96@gmail.com>

pull-request-size Bot added size/XXL and removed size/L labels Jan 28, 2025

github-actions Bot added size/L plugins and removed size/XXL labels Jan 28, 2025

pull-request-size Bot added size/XXL and removed size/L labels Jan 28, 2025

hainenber added 4 commits January 28, 2025 16:10

fix(ci): remove obsolete tsconfig option

4e2921d

Signed-off-by: hainenber <dotronghai96@gmail.com>

fix(ci): add test coverage for CustomControlItem type guard

0bb6363

Signed-off-by: hainenber <dotronghai96@gmail.com>

chore: refactor literal type with OnlyKeyWithType for type-specific v…

3eb10a8

…alue assignment Signed-off-by: hainenber <dotronghai96@gmail.com>

fix(ci): resolve all tsc failures

efde789

Signed-off-by: hainenber <dotronghai96@gmail.com>

hainenber marked this pull request as ready for review January 28, 2025 13:54

hainenber requested review from geido, kgabryje and michael-s-molina as code owners January 28, 2025 13:54

dosubot Bot added change:frontend Requires changing the frontend frontend:refactor Related to refactoring the frontend labels Jan 28, 2025

korbit-ai Bot reviewed Jan 28, 2025

View reviewed changes

michael-s-molina reviewed Jan 28, 2025

View reviewed changes

Comment thread superset-frontend/packages/superset-ui-chart-controls/src/types.ts Outdated

Update superset-frontend/packages/superset-ui-chart-controls/src/type…

68b3141

…s.ts from Michael's suggestion Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

hainenber added 4 commits January 28, 2025 22:27

fix(build): pin Typescript < 5.2 to remove ESLint warning of unsuppor…

fad7f5b

…ted Typescript version Signed-off-by: hainenber <dotronghai96@gmail.com>

chore: address raised functionality issues by Korbit AI

f89eb5f

Signed-off-by: hainenber <dotronghai96@gmail.com>

fix(build): fix all issues after pinning Typescript to v5.1.6

120d0d7

Signed-off-by: hainenber <dotronghai96@gmail.com>

Merge branch 'master' into feat/upgrade-fe-to-typescript-v5

6cb5aa1

Signed-off-by: hainenber <dotronghai96@gmail.com>

Merge branch 'master' into feat/upgrade-fe-to-typescript-v5

0e166c7

Signed-off-by: hainenber <dotronghai96@gmail.com>

michael-s-molina reviewed Jan 28, 2025

View reviewed changes

Comment thread superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts Outdated

Comment thread superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx

chore: add expected type for Array.reduce() function usage

4bf77d3

Signed-off-by: hainenber <dotronghai96@gmail.com>

michael-s-molina removed the review:draft label Jan 28, 2025

michael-s-molina reviewed Jan 28, 2025

View reviewed changes

hainenber added 3 commits January 29, 2025 11:38

chore(fe): migrate ColorSchemeControlWrapper to .tsx

39d2014

Signed-off-by: hainenber <dotronghai96@gmail.com>

chore(fe): refactor getInitialDataMask function for better readability

76861a6

Signed-off-by: hainenber <dotronghai96@gmail.com>

chore(fe): prevent undefined array being args for `fetchObjectsByTags…

49554a8

…Ids` function Signed-off-by: hainenber <dotronghai96@gmail.com>

michael-s-molina approved these changes Jan 29, 2025

View reviewed changes

michael-s-molina merged commit 19e8a70 into apache:master Jan 29, 2025

kgabryje reviewed Jan 29, 2025

View reviewed changes

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 5.0.0 First shipped in 5.0.0 labels Jul 29, 2025

Conversation

hainenber commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

korbit-ai Bot commented Jan 24, 2025

Uh oh!

hainenber commented Jan 28, 2025

Uh oh!

korbit-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

Uh oh!

Uh oh!

Uh oh!

Uh oh!

korbit-ai Bot Jan 28, 2025

Choose a reason for hiding this comment

Incorrect Field Type Casting for helpText

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

Uh oh!

hainenber Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai Bot Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

hainenber Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai Bot Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

michael-s-molina Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai Bot Jan 28, 2025

Choose a reason for hiding this comment

Incorrect Type Union in Native Filters Reduction

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

Uh oh!

hainenber Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai Bot Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hainenber commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-s-molina commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hainenber commented Jan 28, 2025

Uh oh!

hainenber commented Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hainenber commented Jan 24, 2025 •

edited

Loading

korbit-ai Bot left a comment •

edited

Loading

hainenber commented Jan 28, 2025 •

edited

Loading

michael-s-molina commented Jan 28, 2025 •

edited

Loading