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

Tech/introduce-preview-api #19749

Merged
merged 63 commits into from
Nov 24, 2022
Merged

Tech/introduce-preview-api #19749

merged 63 commits into from
Nov 24, 2022

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Nov 3, 2022

Refactor the preview packages

Introduces a new package called preview-api which should be the only package the user would ever want to import in any code that runs on the preview side.

This replaces the need to ever import from @storbook/addons, @storybook/preview-web, @storybook/client-api,@storybook/store,@storybook/core-client.

All these packages are unified under this 1 package.

Likely these packages listed will be removed at some point in the future, simplifying maintenance & future development. It also means users will have an easier time importing the right api from the right place.

I've updated all our code to use the new package.

This should all be backwards compatible, if a user were to still import @storybook/client-api for example they'd see a deprecation warning in the console of their browser, but everything should work.

Some of the code of these packages has actually moved, that includes these 4 packages: client-api, core-client, preview-web and store. Other packages still have their code in the original place, and the preview-api package simply re-exports some or all exports from the original.

The intent is to move all code for the preview into this package when it's not shared anywhere else (runs in the preview only). I decided to make this PR with 4 packages moved, to show it's possible, demonstrate backwards compatibility, and get approval from the other maintainers / rest of the team.

An important step I'm still keen on making is is to split @storybook/addons into 2 packages, 1 for the preview (code should be moved to @storybook/preview-api and the other part for code that runs in the manager (mostly for registering addon panels/etc.) into the @storybook/api package... which I intent to rename to @storybook/manager-api.

Diagram of packages (note that manager-api is currently called api, and this is a proposed change not included in this PR)
Untitled Diagram drawio (7)

This is what prebundling really means:
Untitled Diagram drawio (9)

At the builder level we break the dependency graph into pieces. The are pieces that are stories, and there are pieces that is storybook's runtime code. By breaking the storybook runtime code away from the work that the builder has to do, we gain better performance, because there is less work for the builder to do. The user never has their builder compile storybook's code.

SO the package re-architecture (right now) looks like this:
Untitled Diagram drawio (6)
I've moved code of the 4 packages listed TO preview-api, and for backwards compatibility those packages re-export from preview-api.
@storybook/addons is currently doing something slightly different, because of it's dual use in both the preview and manager (which is a problem) that I will still do. Possibly as part of this PR.

@ndelangen ndelangen self-assigned this Nov 4, 2022
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Nov 4, 2022
@ndelangen ndelangen changed the base branch from next to norbert/sb-798-figure-out-plan-for-package-structure November 4, 2022 12:27
@ndelangen ndelangen changed the base branch from norbert/sb-798-figure-out-plan-for-package-structure to norbert/sb-798-figure-out-plan-for-package-structure-rework November 4, 2022 12:27
code/lib/preview-api/README.md Outdated Show resolved Hide resolved
code/lib/preview-api/src/modules/store/decorators.test.ts Outdated Show resolved Hide resolved
@@ -92,7 +92,7 @@ const run = async ({ cwd, flags }: { cwd: string; flags: string[] }) => {
c.platform = platform || 'browser';
c.legalComments = 'none';
c.minifyWhitespace = optimized;
c.minifyIdentifiers = optimized;
c.minifyIdentifiers = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you leave a comment, why this change is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

See here: https://circleci.com/gh/storybookjs/storybook/444855

When we build our code for dist, we used to compress our variable names.
Turns out when generating variables names like this _2 is a valid identifier.
But when you try and create a function with that name using eval or new Function() it throws.

I decided that not compressing variable names would probably be the right choice here, considering this might make debugging way way easier too.

@tmeasday
Copy link
Member

@ndelangen can all these nice pictures end up in a README or something please?

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.

Love it @ndelangen !

code/lib/preview-api/README.md Outdated Show resolved Hide resolved
@ndelangen ndelangen merged commit 557c55e into next Nov 24, 2022
@ndelangen ndelangen deleted the tech/introduce-preview-api branch November 24, 2022 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:merged Run the CI jobs that normally run when merged. maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants