-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[ui] Improve typechecking on qs usage #29083
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
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 1dfd9ce. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 1dfd9ce. |
@@ -28,10 +28,10 @@ interface ActiveSuggestionInfo { | |||
idx: number; | |||
} | |||
|
|||
export interface TokenizingFieldValue { | |||
export type TokenizingFieldValue = { |
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 made a few changes like this to ensure that the type arguments specified for useQueryPersistedState
etc can satisfy QueryPersistedDataType
.
if (Array.isArray(openNodes)) { | ||
return new Set(openNodes.map((node) => String(node))); | ||
} | ||
return new Set(); |
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.
This is our bug case. We now explicitly check that the value provided via qs
is an array, and cast the contents to string
to match the expected return type.
js_modules/dagster-ui/packages/ui-core/src/assets/useAssetDefinitionFilterState.oss.tsx
Show resolved
Hide resolved
@@ -239,7 +244,7 @@ describe('useQueryPersistedState', () => { | |||
}); | |||
await userEvent.click(screen.getByText(`{"enableA":false,"enableB":true}`)); | |||
await waitFor(() => { | |||
expect(querySearch).toEqual(''); | |||
expect(querySearch).toEqual('?enableA=true&enableB=false'); |
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.
Not sure why this was expecting empty string before...
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.
Hmm this is interesting actually - I think the reason is because the values were being set back to the default specified on like 225, and default values were omitted from the query string. I wonder if the defaults are sometimes used as the query-ified form and sometimes used as the state-form, so changing them to false
above broke this?
| string | ||
| undefined | ||
| number | ||
| boolean | ||
| null; |
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.
The idea here is to more or less match the recursiveness of what qs
is able to encode, and clean up these any
types.
js_modules/dagster-ui/packages/ui-core/src/hooks/useQueryPersistedFilterState.tsx
Show resolved
Hide resolved
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!
35d82c0
to
76467fa
Compare
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.
Ahh this is great, I am definitely guilty of not thinking of the string[] vs string problem...
@@ -239,7 +244,7 @@ describe('useQueryPersistedState', () => { | |||
}); | |||
await userEvent.click(screen.getByText(`{"enableA":false,"enableB":true}`)); | |||
await waitFor(() => { | |||
expect(querySearch).toEqual(''); | |||
expect(querySearch).toEqual('?enableA=true&enableB=false'); |
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.
Hmm this is interesting actually - I think the reason is because the values were being set back to the default specified on like 225, and default values were omitted from the query string. I wonder if the defaults are sometimes used as the query-ified form and sometimes used as the state-form, so changing them to false
above broke this?
76467fa
to
24931c8
Compare
.filter((s): s is AssetPartitionStatus => | ||
DISPLAYED_STATUSES.includes(s as AssetPartitionStatus), |
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.
The type guard function contains a type assertion (s as AssetPartitionStatus
) inside the guard itself, which undermines the purpose of the type guard. This pattern could allow invalid values to pass type checking if the assertion succeeds but the actual value isn't a valid AssetPartitionStatus
.
Consider refactoring to a more reliable approach, such as:
.filter((s): s is AssetPartitionStatus =>
typeof s === 'string' && DISPLAYED_STATUSES.includes(s as AssetPartitionStatus)
)
Or using a string literal union check if AssetPartitionStatus
is an enum or string union type.
.filter((s): s is AssetPartitionStatus => | |
DISPLAYED_STATUSES.includes(s as AssetPartitionStatus), | |
.filter((s): s is AssetPartitionStatus => | |
typeof s === 'string' && DISPLAYED_STATUSES.includes(s as AssetPartitionStatus), |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
[INTERNAL_BRANCH=dish/qs-in-cloud]
24931c8
to
1dfd9ce
Compare
Merge activity
|
Summary & Motivation
Following the recent JS error involving parsed querystrings crashing the page, try to lock down types on our
qs
usage inuseQueryPersistedState
and similar hooks that make use of encode/decode behavior.This PR makes use of the
qs.ParsedQs
type, which is the type returned byqs.parse
itself. This replaces our current{[key: string]: any}
type, which at present leaves us with no real recourse for typechecking our parsed values.Now, encode/decode functions will have to be much more careful about the types involved. For instance, in the case of the error hotfixed by #28791, the
decode
function would now have to refine the value forqs['open-nodes']
in order to satisfy theSet<string>
type in its return value. Ifqs['open-nodes']
were an object (as in the error case), thenew Set(...)
would fail typechecking, and the developer would be forced to provide it an array of strings extracted from theqs
argument.How I Tested These Changes
TS, lint, jest.