Vue3: Add support for Global Apps install #23772
Conversation
code/renderers/vue3/src/index.ts
Outdated
| module.hot.decline(); | ||
| } | ||
| } catch (e) { | ||
| console.log(' e:', e); |
There was a problem hiding this comment.
Do we need to log this?
There was a problem hiding this comment.
not need to log but we need to wrap it in try catch. , it will break in vite ( it is webpack specific )
There was a problem hiding this comment.
Okay, let's remove the console.log then
code/renderers/vue3/src/render.ts
Outdated
|
|
||
| async function installGlobalPlugins(app: App<any>, storyContext: StoryContext<VueRenderer>) { | ||
| if (window.APPLY_PLUGINS_FUNC) { | ||
| await window.APPLY_PLUGINS_FUNC(app, storyContext); |
There was a problem hiding this comment.
What is the difference with the setup function? Should this function be set by the user?
There was a problem hiding this comment.
using setup is not safe, in case your package manager allows 2 versions of the same package, @storybook/vue3 can't be a singleton, so calling setup. seems not working for users
using window. APPLY_PLUGINS_FUNC will insure the singleton.
it should be. awaited as well in case. registering the plugin is async
There was a problem hiding this comment.
I think we can also solve this by putting this on the window:
I think it is better have one API.
There was a problem hiding this comment.
yes exactly i'm working on that idea
| // optimization: stop HMR propagation in webpack | ||
| if (typeof module !== 'undefined') module?.hot?.decline(); | ||
| try { | ||
| if (module?.hot?.decline) { | ||
| module.hot.decline(); | ||
| } |
There was a problem hiding this comment.
You still need the typeof module !== 'undefined' check right?
Maybe:
if (typeof module !== 'undefined') module?.hot?.decline?.();| globalThis.PLUGINS_SETUP_FUNCTIONS ??= new Set(); | ||
| globalThis.PLUGINS_SETUP_FUNCTIONS.add(fn); |
There was a problem hiding this comment.
@ndelangen I thought you mentioned at some point that we prefer @storybook/global over globalThis but I don't know the reasons anymore.
There was a problem hiding this comment.
as i know globalThis was introduced to give access to global scope regardless the system Node/Browser, it is consistency and standard rather than using window / global
There was a problem hiding this comment.
and that what @storybook/global provides the global scope so if we do
declare var STORYBOOK_ENV:'type' in type.d.ts file
will be safely available on globalThis as global module
| if (globalThis && globalThis.PLUGINS_SETUP_FUNCTIONS) | ||
| await Promise.all([...globalThis.PLUGINS_SETUP_FUNCTIONS].map((fn) => fn(app, storyContext))); |
There was a problem hiding this comment.
Maybe Promise.allSettled as I think we don't want to short-circuit here?
code/renderers/vue3/src/render.ts
Outdated
| const runSetupFunctions = async ( | ||
| app: App, | ||
| storyContext: StoryContext<VueRenderer> | ||
| ): Promise<any> => { |
Closes #23844
What I did
How to test
Checklist
MIGRATION.MD
Maintainers
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]馃 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>