-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Embed widget loading workspace settings #2068
Conversation
return { | ||
listItems: jest.fn(async () => []), | ||
loadFile: jest.fn(async () => { | ||
throw new Error('No file loaded'); | ||
}), | ||
deleteItem: jest.fn(async () => undefined), | ||
saveFile: jest.fn(async () => undefined), | ||
saveFile: jest.fn( |
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 might be a good spot for
TestUtils.createMockProxy<dh.storage.StorageService>({
loadFile: jest.fn(async () => {
throw new Error('No file loaded');
}),
})
@@ -60,6 +88,43 @@ function App(): JSX.Element { | |||
throw new Error('Missing URL parameter "name"'); | |||
} | |||
|
|||
const sessionDetails = await getSessionDetails(); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Left 2 suggestions. Nothing big enough to block the PR
@@ -60,6 +88,43 @@ function App(): JSX.Element { | |||
throw new Error('Missing URL parameter "name"'); | |||
} | |||
|
|||
const sessionDetails = await getSessionDetails(); |
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.
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.
Fixes #1964. Also removed a bunch of unnecessary code in
AppInit
where we were using the reduxconnect
API 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-studio
is 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>