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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 28 commits into from
Nov 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6538477
add feature for fast-build based on `--test` CLI flag
ndelangen Nov 1, 2023
9a72ed0
globalize `@storybook/blocks` if `fastBuildEmptyBlocks` is set to true
ndelangen Nov 1, 2023
dd598b4
Merge branch 'next' into norbert/fast-build-toggles
ndelangen Nov 1, 2023
bbcf2bc
add test flag to CLIoptions add fastBuildEmptyBlocks to features inte…
ndelangen Nov 1, 2023
fc00057
Merge branch 'norbert/fast-build-toggles' of github.com:storybookjs/s…
ndelangen Nov 1, 2023
e157a2a
Merge branch 'next' into norbert/fast-build-toggles
ndelangen Nov 2, 2023
d241d03
Simplify globals type, so that it is mutable
kasperpeulen Nov 2, 2023
533d728
move from using `features` to useing a new property in presets called…
ndelangen Nov 2, 2023
af3225f
flip the test.build around
ndelangen Nov 2, 2023
c100c46
forgot to change it in common-preset
ndelangen Nov 2, 2023
9188288
Merge branch 'next' into norbert/fast-build-toggles
ndelangen Nov 2, 2023
8c2d720
swap it again
ndelangen Nov 2, 2023
8fbe192
Merge branch 'norbert/fast-build-toggles' of github.com:storybookjs/s…
ndelangen Nov 2, 2023
52e38c6
add the global for `__STORYBOOK_BLOCKS_EMPTY_MODULE__` if needed.
ndelangen Nov 2, 2023
33ec86a
make it optional
ndelangen Nov 2, 2023
40ea412
ignore eslint warning, deal with optionality
ndelangen Nov 2, 2023
5230210
adjust the eslint config to actually do it
ndelangen Nov 2, 2023
665ddee
oops
ndelangen Nov 2, 2023
901516b
fix
ndelangen Nov 2, 2023
dda1af7
fix
ndelangen Nov 2, 2023
cd3ad1f
fix
ndelangen Nov 2, 2023
1a90573
Keep known globals as type, but allow extension
kasperpeulen Nov 3, 2023
ebd4fa6
Upload test build to chromatic
kasperpeulen Nov 3, 2023
5c51f63
Update parallelism
kasperpeulen Nov 3, 2023
1c70ab2
fix
ndelangen Nov 3, 2023
c3e78ac
Merge branch 'norbert/fast-build-toggles' of github.com:storybookjs/s…
ndelangen Nov 3, 2023
d64b883
Merge branch 'next' into norbert/fast-build-toggles
ndelangen Nov 3, 2023
7c7661a
Rollback my type fix, but also remove the ts-expect-errors
kasperpeulen Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions code/builders/builder-vite/src/vite-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ export async function commonConfig(

export async function pluginConfig(options: Options) {
const frameworkName = await getFrameworkName(options);
const { features } = options;
ndelangen marked this conversation as resolved.
Show resolved Hide resolved

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

@JReinhold JReinhold Nov 2, 2023 •
edited
Loading

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 •
edited
Loading

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!

}

const plugins = [
codeGeneratorPlugin(options),
Expand Down
4 changes: 4 additions & 0 deletions code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ export default async (
`);
}

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

return {
name: 'preview',
mode: isProd ? 'production' : 'development',
Expand Down
8 changes: 7 additions & 1 deletion code/lib/core-server/src/presets/common-preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,21 @@ export const previewAnnotations = async (base: any, options: Options) => {
return [...config, ...base];
};

const fastBuildFeatures = (value: boolean) => ({
ndelangen marked this conversation as resolved.
Show resolved Hide resolved
fastBuildEmptyBlocks: value,
});

export const features = async (
existing: StorybookConfig['features']
existing: StorybookConfig['features'],
options: Options
): Promise<StorybookConfig['features']> => ({
...existing,
warnOnLegacyHierarchySeparator: true,
buildStoriesJson: false,
storyStoreV7: true,
argTypeTargetsV7: true,
legacyDecoratorFileOrder: false,
...fastBuildFeatures(!!options.test),
ndelangen marked this conversation as resolved.
Show resolved Hide resolved
});

export const csfIndexer: Indexer = {
Expand Down
2 changes: 1 addition & 1 deletion code/lib/preview/src/globals/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?

'@storybook/addons': '__STORYBOOK_MODULE_ADDONS__',
'@storybook/global': '__STORYBOOK_MODULE_GLOBAL__',
'@storybook/channel-postmessage': '__STORYBOOK_MODULE_CHANNEL_POSTMESSAGE__', // @deprecated: remove in 8.0
Expand Down
6 changes: 6 additions & 0 deletions code/lib/types/src/modules/core-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,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 •
edited
Loading

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 •
edited
Loading

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?

debugWebpack?: boolean;
webpackStatsJson?: string | boolean;
outputDir?: string;
Expand Down Expand Up @@ -310,6 +311,11 @@ export interface StorybookConfig {
* Apply decorators from preview.js before decorators from addons or frameworks
*/
legacyDecoratorFileOrder?: boolean;

/**
* Globalize @storybook/blocks
*/
fastBuildEmptyBlocks?: boolean;
};

/**
Expand Down