Skip to content
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

Fix: vite devmode with storyStoreV6 by ensuring singleton via global #20207

Merged

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Dec 12, 2022

Related: #20184

What I did

Ensure we only create a single instance of ClientApi

How to test?

  • manually change code/ui/.storybook.ts so it uses storyStoreV7: false
  • run yarn storybook:uiin code.

I tested this locally.

  • expect storybook to open, and become populated with stories correctly

@ndelangen ndelangen self-assigned this Dec 12, 2022
@ndelangen ndelangen added the bug label Dec 12, 2022
@ndelangen ndelangen changed the title ensure singleton via global Fix: vite devmode with storyStoreV6 by ensuring singleton via global Dec 12, 2022
@ndelangen ndelangen requested a review from IanVS December 12, 2022 17:09
@ndelangen ndelangen requested a review from tmeasday December 12, 2022 19:09
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we need to do this @ndelangen?

(a) for v6: I would expect that loading the start.js file several times would be bad news even if the preview is "singleton-ized" (e.g. we would potentially add the user's decorators etc more than once)

(b) for v7: if this is a bug in v6, why do we need to update the modern entries? I don't love the idea of writing the code to expect it to be loaded >1 times when we don't want it to be.

@ndelangen
Copy link
Member Author

@tmeasday we need to do this, because unfortunately there's some bug in the vite dev mode, that makes it skip transforming a file. I figured I'd do the change on both, to avoid the future question of: "why is this only done for a, and not for b?" Make the change more uniform, so it's less unexpected.

start is never called multiple times, because the start invocation isn't part of the prebundle, that's in the renderer.
So I am sure double decorator loading is not an issue.

Maybe this isn't a problem with Vite4? I'd need to try, but I think ensuring these things to be singletons isn't the worst we could do.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed we can drop the changes to all the entry points and mostly confine this to ClientApi.ts.

The rationale is as follows:

  • The entry points / start.ts can only run once no matter what. If they ran more than once, there'd be major problems with multiple channels etc.
  • Those entry points create the "real" client api / preview and set it on global scope.
  • In the v6 store, we also evaluate previewAnnotation files (e.g. preview.js) and call eg. addParameters() from ClientApi. The problem comes when that version of ClientApi is wrong, and it's singleton is not initialized. However if it reads the singleton from global scope, like you do in this PR, we bypass the issue, as the singleton was already set on global scope by the entrypoint.

@ndelangen ndelangen force-pushed the norbert/sb-1063-vite-in-dev-mode-with-storystorev6-cleaned branch from 91fe4d8 to 775f3ae Compare December 13, 2022 11:19
@ndelangen ndelangen requested a review from tmeasday December 13, 2022 11:19
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@ndelangen ndelangen merged commit 8063a0e into next Dec 13, 2022
@ndelangen ndelangen deleted the norbert/sb-1063-vite-in-dev-mode-with-storystorev6-cleaned branch December 13, 2022 16:41
@tmeasday
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants