feat: Embed widget loading workspace settings#2068
feat: Embed widget loading workspace settings#2068mattrunyon merged 4 commits intodeephaven:mainfrom
Conversation
| }), | ||
| deleteItem: jest.fn(async () => undefined), | ||
| saveFile: jest.fn(async () => undefined), | ||
| saveFile: jest.fn( |
There was a problem hiding this comment.
This might be a good spot for
TestUtils.createMockProxy<dh.storage.StorageService>({
loadFile: jest.fn(async () => {
throw new Error('No file loaded');
}),
})
| throw new Error('Missing URL parameter "name"'); | ||
| } | ||
|
|
||
| const sessionDetails = await getSessionDetails(); |
There was a problem hiding this comment.
There seems to be a lot of duplicate code here as AppInit initApp effect. Any chance we could split out the common code into a useInitApp hook or something?
There was a problem hiding this comment.
Probably. I think I'd name it useInitRedux or something like that.
There were a few I omitted since they aren't needed in embed-widget (like CommandHistory). I initially had most of the new code in a separate effect, but it actually caused a race condition with loading a table because the widget would get fetched and start rendering, but the user hadn't been set in redux yet.
I did also try moving some of the redux setters to their respective *Bootstrap components. But would need to hoist the redux store up since right now it's a child of all the bootstraps. Shouldn't have an impact though if that's what we went with
There was a problem hiding this comment.
Instead of hook could just be an async helper function that gets called with all these params passed in, then await initializeApp(...) here; or a Bootstrap component as you suggested. Either way this is fine for now.
There was a problem hiding this comment.
True. Either way pulling it out then requires moving some redux things out of code-studio and I think this is fine as is for now
|
Tested and confirmed this works as described. |
bmingles
left a comment
bmingles
left a comment
There was a problem hiding this comment.
Left 2 suggestions. Nothing big enough to block the PR
| throw new Error('Missing URL parameter "name"'); | ||
| } | ||
|
|
||
| const sessionDetails = await getSessionDetails(); |
There was a problem hiding this comment.
Instead of hook could just be an async helper function that gets called with all these params passed in, then await initializeApp(...) here; or a Bootstrap component as you suggested. Either way this is fine for now.
3 participants
Fixes #1964. Also removed a bunch of unnecessary code in
AppInitwhere we were using the reduxconnectAPI with a functional component instead of just using the redux hooksNote that even though this moves a bunch of files to
app-utils, I don't consider it breaking since they are being moved out ofcode-studio. They wouldn't be consumed by anything else sincecode-studiois published as a bundle.Due to local storage requiring a port match, this can only be tested w/ the local server in 2 steps (this should be fine in prod since the app and embed-widget run on the same port there).
PORT=4000 npm run start:embed-widget. This will let us access the saved workspace from the applocalhost:4000/?name=<your-variable-name>