[ui] Lint against window.open #25912
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit fa02329. |
| @@ -1,12 +1,16 @@ | |||
| import {useContext} from 'react'; | |||
| import {useCallback, useContext} from 'react'; | |||
|
|
|||
| import {AppContext} from '../app/AppContext'; | |||
|
|
|||
| // Open a path in a new tab. Incorporates the base path to ensure that | |||
| // the link opens to the appropriate deployment. | |||
| export const useOpenInNewTab = () => { | |||
There was a problem hiding this comment.
Does this need to be a hook? Would prefer a regular function. Maybe we can move the basePath to be returned by extractCloudDagsterUISettings if it doesn't already have enough information to obtain it?
There was a problem hiding this comment.
So currently we pass it to AppContext, maybe we can just use a global instead?
There was a problem hiding this comment.
Yeah, I think we could do that. It seemed to me that a hook was fine since we're unlikely to be executing window.open outside of a React context, but I don't feel strongly about it.
There was a problem hiding this comment.
I think sometimes you might have logic that is divorced from React (eg: inside some helper function 2 levels deep) and it feels weird to require a component to thread the openInNewTab function that deep. It requires every function along the path to know that this functionality is nested inside which seems like a bad separate of concerns to me.
hellendag
force-pushed
the
dish/window-open-lint
branch
from
0ee85cb
to
fa02329
Compare
Summary & Motivation
Stacking on #25909, lint against using
window.open. This is because our current usage generally ignores anybasePathvalue that might be in place in the app, e.g. a deployment path prefix in Dagster+.The result is that opening the new tab may lead to a redirect that opens the path in a default fallback deployment, instead of the viewer's current deployment.
To avoid this,
useOpenInNewTabprepends the basePath to the path value. I have added a lint rule to error onwindow.openusage and push users toward this hook instead.How I Tested These Changes
TS, lint, jest.