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

Presets: Support recursive addon imports #19176

Closed
shilman opened this issue Sep 13, 2022 · 6 comments
Closed

Presets: Support recursive addon imports #19176

shilman opened this issue Sep 13, 2022 · 6 comments

Comments

@shilman
Copy link
Member

shilman commented Sep 13, 2022

Currently, addons can reference other addons by name. For example, addon-essentials has the following structure:

export const addons = async (existing) => ([...existing, '@storybook/addon-actions', ...]);

This matches how users configure addons in their .storybook/main.js file. However, it presents problems when it comes to package management / module resolution / hoisting, since the dependencies are implicit. If instead we supported the ability to reference other addons directly, this would mean that an addon's dependencies are explicit and we wouldn't have to play any games with module resolution.

We should update Storybook's preset mechanism and port existing addons to this structure:

export const addons = async (existing) => ([...existing, await import('@storybook/addon-actions'), ...]);

We wouldn't need to force users to adopt this in main.js, but it would be useful for addons that make use of other addons (e.g. addon-essentials).

@IanVS
Copy link
Member

IanVS commented Sep 13, 2022

👍 This will help us to improve support for package managers that don't hoist, like pnpm and yarn pnp without some of the games we currently play like using require.resolve() to get an absolute path (which also always resolves to the cjs version, even if esm could be used).

@IanVS
Copy link
Member

IanVS commented Oct 12, 2022

Is this something that I could start working on, or do you envision it being something with sharp edges, @shilman @ndelangen?

Edit: I'm giving it a shot. So far the trickiest part is figuring out how the previewAnnotations, managerEntries, and presets which are normally absolute paths, are actually used later on, so that I can adjust it to account for sometimes having the actual code, and sometimes just the path (trying to keep this backwards-compatible).

@IanVS
Copy link
Member

IanVS commented Oct 19, 2022

OK, I've gotten further. I can now require the sub-addons just fine, but I think there's a fatal flaw here. I now have an array of previewAnnotations that is something like this:

{
  previewAnnotations: [
    '@storybook/react/preview',
    '@storybook/addon-links/preview',
    { parameters: [Getter] },
    { argsEnhancers: [Getter], decorators: [Getter] },
    { decorators: [Getter], parameters: [Getter] },
    { decorators: [Getter], globals: [Getter] },
    { decorators: [Getter], globals: [Getter] },
    { highlightObject: [Getter], highlightStyle: [Getter] },
    '@storybook/addon-interactions/preview',
    './src/stories/components',
    './template-stories/lib/store/preview.ts',
    './template-stories/addons/toolbars/preview.ts'
  ]
}

Now, I need to get that into the preview. But since there are objects with functions, I guess I can't just serialize it straight into the virtual module in the preview builder. The strings can be imported as before, but what can I do with the sub-addons after I've gotten them in node? 😕

I've opened a draft PR in case anyone wants to take a look: #19530

@shilman
Copy link
Member Author

shilman commented Nov 13, 2022

Hurrah!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.49 containing PR #19689 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Nov 13, 2022
@shilman
Copy link
Member Author

shilman commented Nov 13, 2022

@IanVS @tmeasday is it correct that we won't pursue this issue now that @IanVS 's PR has been merged/released, and if we do enhance it, it will be a different approach with an addons field in preview.js? In other words, should I leave this closed?

@tmeasday
Copy link
Member

That sounds right to me @shilman

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

No branches or pull requests

3 participants