Suspensify PermissionsProvider using useWrappedQuery
#21440
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit f43e10c. |
| </Route> | ||
| <Route path="/assets(/?.*)"> | ||
| <Suspense fallback={<div />}> | ||
| <TrackedSuspense fallback={<div />} id={id('assets')}> |
There was a problem hiding this comment.
I have two questions here:
-
Why is it necessary to put the Suspense inside the Route and not inside the Switch on line 37? Does this really need to be repeated for every route separately like this? Is this primarily so the
idcan be passed? -
If the reason for Separate Pandas Dataframe Solid into Two Sources #1 is so that the suspense instance is replaced when you navigate, do these
/*type routes all share the same tracked suspense? Is that ok?
There was a problem hiding this comment.
It's not necessary, I was just replacing the existing Suspense calls with TrackedSuspense in a follow up we can consolidate these but didn't want to unknowingly break anything by doing that
| showCustomAlert({title: 'Error', body: <PythonErrorInfo error={backfill.error} />}) | ||
| } | ||
| onClick={() => { | ||
| console.log('backfill error'); |
| onClick={() => { | ||
| console.log('backfill error'); | ||
| if (backfill.error) { | ||
| console.log('Show custom alert'); |
| rawUnscopedData: PermissionFragment[]; | ||
| }; | ||
|
|
||
| type PermissionsContextType = ReturnType<typeof wrapPromise<PermissionsResult>>; |
There was a problem hiding this comment.
Would be nice for your wrapPromise util to expose a type helper that makes this Wrapped<PermissionsResult>, I expect we'll need this in a lot of places
| async (result) => { | ||
| const {data, error} = result; | ||
|
|
||
| const value = React.useMemo(() => { | ||
| const unscopedPermissionsRaw = data?.unscopedPermissions || []; | ||
| const unscopedPermissions = extractPermissions(unscopedPermissionsRaw); | ||
| const unscopedPermissionsRaw = data?.unscopedPermissions || []; | ||
| const unscopedPermissions = extractPermissions(unscopedPermissionsRaw); | ||
|
|
||
| const locationEntries = | ||
| data?.workspaceOrError.__typename === 'Workspace' | ||
| ? data.workspaceOrError.locationEntries | ||
| : []; | ||
| const locationEntries = | ||
| data?.workspaceOrError.__typename === 'Workspace' | ||
| ? data.workspaceOrError.locationEntries | ||
| : []; | ||
|
|
||
| const locationPermissions: Record<string, PermissionsMap> = {}; | ||
| locationEntries.forEach((locationEntry) => { | ||
| const {name, permissions} = locationEntry; | ||
| locationPermissions[name] = extractPermissions(permissions, unscopedPermissionsRaw); | ||
| }); | ||
| const locationPermissions: Record<string, PermissionsMap> = {}; | ||
| locationEntries.forEach((locationEntry) => { | ||
| const {name, permissions} = locationEntry; | ||
| locationPermissions[name] = extractPermissions(permissions, unscopedPermissionsRaw); | ||
| }); | ||
|
|
||
| return { | ||
| unscopedPermissions, | ||
| locationPermissions, | ||
| loading, | ||
| rawUnscopedData: unscopedPermissionsRaw, | ||
| }; | ||
| }, [data, loading]); | ||
| return { | ||
| error, | ||
| unscopedPermissions, | ||
| locationPermissions, | ||
| rawUnscopedData: unscopedPermissionsRaw, | ||
| }; |
There was a problem hiding this comment.
This all looks good to me! Just from reading this it's not clear that the use of useWrapped has suspense implications, it sorta just looks like a stylistic change moving the data transform into the hook. Maybe we call out in the naming that this significantly changes the loading semantics.
Summary & Motivation
Suspensify
PermissionsProvider. Concretely this means instead of returning PermissionsResults, it instead returns a wrappedPromise (an object with areadmethod that suspends (throws a promise) if the data is not available).The idea here is that we don't want PermissionsProvider itself to suspend because the route being rendered might not use the permission data at all, and because the route being rendered might want to make its own queries in parallel and if we suspended immediately it wouldn't get the chance to do that. Instead a child component reads the data after making their own suspended query and then calls the
readfunction which would cause it to suspend.How I Tested These Changes
Local dagster dev, navigate the app and make sure things that check permissions (like the assets table) are still correctly obtaining permissions