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

TestBuild: Globalize @storybook/blocks if build.test.emptyBlocks is true #24650

Merged
merged 28 commits into from
Nov 3, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Nov 1, 2023

What I did

The recently introduced --test flag currently doesn't toggle much functionality.

This introduces the first change in functionality based on this flag:

  • When the flag is true an build.test.emptyBlocks becomes true (by default, can be switched back to false in main.ts).
  • This causes the preview builders to globalize @storybook/blocks.
  • The effect of that is that the builder can skip those imports, thus making the bundle-process faster and the generated storybook-static's size smaller.
  • It will cause actual usage of @storybook/blocks to instantly fail.

The idea is that --test will gain more feature flags like these that eliminate docs completely.
This is just 1 small step in that direction.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  • Create a sandbox
  • Run sb build --test in the sandbox
  • Check if @storybook/blocks exists/is globalized

Do the above for both a webpack based and vite based sandbox.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 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/core team 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>

@ndelangen ndelangen changed the title globalize @storybook/blocks if fastBuildEmptyBlocks is set to true FastBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is set to true Nov 1, 2023
@ndelangen ndelangen changed the title FastBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is set to true FastBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is true Nov 1, 2023
const { features } = options;

if (features?.fastBuildEmptyBlocks) {
globals['@storybook/blocks'] = '__STORYBOOK_BLOCKS_EMPTY_MODULE__';
Copy link
Contributor

@JReinhold JReinhold Nov 2, 2023

Choose a reason for hiding this comment

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

This is a bit of a hack, reusing the externalGlobalsPlugin for this purpose.

It would be more "correct" and easier to understand later to create a separate plugin for this that did

// before
import { Primary, Stories } from '@storybook/blocks';

//after
const { Primary, Stories } = {};

effectively it's the same, but I think this might be easier to debug down the line. It's also more effort though, which might not be worth it right now.

Copy link
Contributor

@JReinhold JReinhold Nov 2, 2023

Choose a reason for hiding this comment

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

On second thought, I don't understand why this PR even works in it's current state. Based on what I see in that plugin, it will now do:

// before
import { Primary, Stories } from '@storybook/blocks';

//after
const { Primary, Stories } = __STORYBOOK_BLOCKS_EMPTY_MODULE__;

But if __STORYBOOK_BLOCKS_EMPTY_MODULE__ is undefined that line will surely fail to destructure?

Copy link
Contributor

Choose a reason for hiding this comment

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

It gets assigned an empty object in the builders!

@ndelangen ndelangen changed the title FastBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is true TestBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is true Nov 2, 2023
@ndelangen ndelangen changed the title TestBuild: Globalize @storybook/blocks if fastBuildEmptyBlocks is true TestBuild: Globalize @storybook/blocks if test.build.emptyBlocks is true Nov 2, 2023
@@ -1,5 +1,5 @@
// Here we map the name of a module to their NAME in the global scope.
export const globals = {
export const globals: Record<string, string> = {
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea here to allow referencing globals that don't actually exist in this list? That seems a little bit dangerous, as it will remove some type safety when referencing them throughout the codebase. I think it would be better to add a //@ts-expect-error when globals['@storybook/blocks'] is used, instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@IanVS What do you think of this compromise?

const _globals = {
  '@storybook/addons': '__STORYBOOK_MODULE_ADDONS__',
  '@storybook/global': '__STORYBOOK_MODULE_GLOBAL__',
  '@storybook/channel-postmessage': '__STORYBOOK_MODULE_CHANNEL_POSTMESSAGE__', // @deprecated: remove in 8.0
  '@storybook/channel-websocket': '__STORYBOOK_MODULE_CHANNEL_WEBSOCKET__', // @deprecated: remove in 8.0
  '@storybook/channels': '__STORYBOOK_MODULE_CHANNELS__',
  '@storybook/client-api': '__STORYBOOK_MODULE_CLIENT_API__',
  '@storybook/client-logger': '__STORYBOOK_MODULE_CLIENT_LOGGER__',
  '@storybook/core-client': '__STORYBOOK_MODULE_CORE_CLIENT__',
  '@storybook/core-events': '__STORYBOOK_MODULE_CORE_EVENTS__',
  '@storybook/preview-web': '__STORYBOOK_MODULE_PREVIEW_WEB__',
  '@storybook/preview-api': '__STORYBOOK_MODULE_PREVIEW_API__',
  '@storybook/store': '__STORYBOOK_MODULE_STORE__',
};

export const globals: typeof _globals & Record<string, string> = _globals;

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sure it is still autocompleted for the known keys:
image

Copy link
Member

Choose a reason for hiding this comment

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

But then it won't start to fail if one of those keys is removed, will it? I think that's a useful property to have, personally. Likewise, if __STORYBOOK_BLOCKS_EMPTY_MODULE__ does get added, we would want the places expecting it to be empty to start to fail (the ts-expect-error).

That said, I don't see any ts-expec-error in the code anymore, so it looks like maybe this is a non issue?

@@ -156,6 +157,7 @@ export interface CLIOptions {
quiet?: boolean;
versionUpdates?: boolean;
docs?: boolean;
test?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

test feels a little bit ambiguous to me. I can imagine a new user looking at these options and wondering what the difference is between test and ci, for example. At the risk of being verbose, I would suggest testMode or testingMode or just testing or to be symmetric with docs: tests.

Copy link
Member Author

@ndelangen ndelangen Nov 2, 2023

Choose a reason for hiding this comment

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

I changed it to build.test as per @shilman's suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@IanVS We had a long internal discussion about this. I'm okay with --test-mode or --testing, but I guess if we have good docs --test should be fine as well.

Copy link
Member

@IanVS IanVS Nov 3, 2023

Choose a reason for hiding this comment

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

Sure, though I don't like relying on docs to make up for confusing names. Not everyone reads docs, and the command may be put into a package.json script where the docs are not at hand when someone is reading it and trying to understand what it does. I kind of like --tests, to match --docs, personally, since they are both special modes of running Storybook, it feels like they should kind of match up.

That said, I don't feel super-strongly about the naming, just wanted to bring it up as a potentially confusing name. Thanks for the consideration!

Copy link
Contributor

Choose a reason for hiding this comment

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

@shilman What do you think about —tests?

@ndelangen ndelangen changed the title TestBuild: Globalize @storybook/blocks if test.build.emptyBlocks is true TestBuild: Globalize @storybook/blocks if build.test.emptyBlocks is true Nov 2, 2023
@kasperpeulen kasperpeulen self-requested a review November 3, 2023 15:36
Copy link
Contributor

@kasperpeulen kasperpeulen 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 cc6d582 into next Nov 3, 2023
57 of 59 checks passed
@ndelangen ndelangen deleted the norbert/fast-build-toggles branch November 3, 2023 15:38
@github-actions github-actions bot mentioned this pull request Nov 3, 2023
30 tasks
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.

5 participants