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

API: Add method to set config for manager #8232

Merged
merged 8 commits into from
Oct 3, 2019
Merged

Conversation

ndelangen
Copy link
Member

Issue:
Setting theme via parameters is really bad for performance
This is true for other config too.

What I did

ADD a setConfig option on lib/addons, to pass data/config into the manager directly VS via parameters

This PR adds a API:

import { addons } from '@storybook/addons';
import { themes } from '@storybook/theming';

addons.setConfig({
  theme: themes.dark,
});

You set this within addons.js.

How to test

I've changed the dev-kits example to dark theme.

There's no flash of light theme anymore.

@ndelangen
Copy link
Member Author

I implemented the setting of theme, but all other configs will still have to be implemented.

Looking forward to hearing your feedback @shilman

@vercel
Copy link

vercel bot commented Sep 30, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-core-setconfig-method.storybook.now.sh

@shilman
Copy link
Member

shilman commented Sep 30, 2019

@ndelangen Super interesting, and raises lots of questions:

  • Are these settings also available in the preview? (And vice versa)
  • Which settings should be set this way vs. the old way?
  • Do we want to rename addons.js to manager.js?
  • What's the migration path for users?
  • Is this a breaking change?

@ndelangen
Copy link
Member Author

ndelangen commented Sep 30, 2019

Are these settings also available in the preview? (And vice versa)

No, parameters are shared meta-data on stories, those are shared

Which settings should be set this way vs. the old way?

If the data is for the manager only, parameters only make sense if the data is related/attached to a story

Do we want to rename addons.js to manager.js?

Would probably be a good change to make, that would be a breaking change though

What's the migration path for users?

We'd need to change the documentation & our examples.
I don't think a codemod is viable. So much stuff can go down in config.js...
We can however add a deprecation and a warning how to change config.js to remove the custom theme, and how to add it to addons.js.

Is this a breaking change?

No, not as it is right now.

@shilman
Copy link
Member

shilman commented Sep 30, 2019

I understand it & it's a pretty cool change! Not exactly sure what's the best way to communicate it to users, but it's a big perf win.

@ndelangen
Copy link
Member Author

Addresses:
#8188

@ndelangen
Copy link
Member Author

It would be nice to get some stricter types on this.

@ndelangen ndelangen merged commit d7495fb into next Oct 3, 2019
@ndelangen ndelangen deleted the core/setConfig-method branch October 3, 2019 10:57
@shilman shilman changed the title ADD method to set config for manager API: Add method to set config for manager Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants