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

[ndelangen]

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>

[JReinhold]

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.

[JReinhold]

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?

[kasperpeulen]

It gets assigned an empty object in the builders!

[IanVS]

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.

[kasperpeulen]

@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;

[kasperpeulen]

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

[IanVS]

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?

[IanVS]

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.

[ndelangen]

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

[kasperpeulen]

@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.

[IanVS]

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!

[kasperpeulen]

@shilman What do you think about —tests?

[kasperpeulen]

LGTM!