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 all 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
6 changes: 3 additions & 3 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ workflows:
requires:
- create-sandboxes
- chromatic-sandboxes:
parallelism: 9
parallelism: 11
requires:
- build-sandboxes
- e2e-production:
Expand Down Expand Up @@ -563,7 +563,7 @@ workflows:
requires:
- create-sandboxes
- chromatic-sandboxes:
parallelism: 18
parallelism: 20
requires:
- build-sandboxes
- e2e-production:
Expand Down Expand Up @@ -620,7 +620,7 @@ workflows:
requires:
- create-sandboxes
- chromatic-sandboxes:
parallelism: 33
parallelism: 35
requires:
- build-sandboxes
- e2e-production:
Expand Down
7 changes: 7 additions & 0 deletions code/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ module.exports = {
'@typescript-eslint/ban-types': 'warn', // should become error, in the future
},
},
{
// this package depends on a lot of peerDependencies we don't want to specify, because npm would install them
files: ['**/builder-vite/**/*.html'],
rules: {
'@typescript-eslint/no-unused-expressions': 'off', // should become error, in the future
},
},
{
// these packages use pre-bundling, dependencies will be bundled, and will be in devDepenencies
files: [
Expand Down
2 changes: 2 additions & 0 deletions code/builders/builder-vite/input/iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
window.STORIES = '[STORIES HERE]';
window.DOCS_OPTIONS = '[DOCS_OPTIONS HERE]';

('OTHER_GLOBLALS HERE');

// We do this so that "module && module.hot" etc. in Storybook source code
// doesn't fail (it will simply be disabled)
window.module = undefined;
Expand Down
11 changes: 11 additions & 0 deletions code/builders/builder-vite/src/transform-iframe-html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export type PreviewHtml = string | undefined;

export async function transformIframeHtml(html: string, options: Options) {
const { configType, features, presets } = options;
const build = await presets.apply('build');
const frameworkOptions = await presets.apply<Record<string, any> | null>('frameworkOptions');
const headHtmlSnippet = await presets.apply<PreviewHtml>('previewHead');
const bodyHtmlSnippet = await presets.apply<PreviewHtml>('previewBody');
Expand All @@ -20,10 +21,20 @@ export async function transformIframeHtml(html: string, options: Options) {
importPathMatcher: specifier.importPathMatcher.source,
}));

const otherGlobals = {
...(build?.test?.emptyBlocks ? { __STORYBOOK_BLOCKS_EMPTY_MODULE__: {} } : {}),
};

return html
.replace('[CONFIG_TYPE HERE]', configType || '')
.replace('[LOGLEVEL HERE]', logLevel || '')
.replace(`'[FRAMEWORK_OPTIONS HERE]'`, JSON.stringify(frameworkOptions))
.replace(
`('OTHER_GLOBLALS HERE');`,
Object.entries(otherGlobals)
.map(([k, v]) => `window["${k}"] = ${JSON.stringify(v)};`)
.join('')
)
.replace(
`'[CHANNEL_OPTIONS HERE]'`,
JSON.stringify(coreOptions && coreOptions.channelOptions ? coreOptions.channelOptions : {})
Expand Down
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 build = await options.presets.apply('build');

if (build?.test?.emptyBlocks) {
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
7 changes: 7 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 @@ -90,6 +90,7 @@ export default async (
entries,
nonNormalizedStories,
modulesCount = 1000,
build,
] = await Promise.all([
presets.apply<CoreConfig>('core'),
presets.apply('frameworkOptions'),
Expand All @@ -102,6 +103,7 @@ export default async (
presets.apply<string[]>('entries', []),
presets.apply('stories', []),
options.cache?.get('modulesCount').catch(() => {}),
options.presets.apply('build'),
]);

const stories = normalizeStories(nonNormalizedStories, {
Expand Down Expand Up @@ -217,6 +219,10 @@ export default async (
`);
}

if (build?.test?.emptyBlocks) {
globals['@storybook/blocks'] = '__STORYBOOK_BLOCKS_EMPTY_MODULE__';
}

return {
name: 'preview',
mode: isProd ? 'production' : 'development',
Expand Down Expand Up @@ -269,6 +275,7 @@ export default async (
importPathMatcher: specifier.importPathMatcher.source,
})),
DOCS_OPTIONS: docsOptions,
...(build?.test?.emptyBlocks ? { __STORYBOOK_BLOCKS_EMPTY_MODULE__: {} } : {}),
},
headHtmlSnippet,
bodyHtmlSnippet,
Expand Down
1 change: 0 additions & 1 deletion code/frameworks/nextjs/src/preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { configureAliasing } from './dependency-map';

export const addons: PresetProperty<'addons', StorybookConfig> = [
dirname(require.resolve(join('@storybook/preset-react-webpack', 'package.json'))),
dirname(require.resolve(join('@storybook/builder-webpack5', 'package.json'))),
];

const defaultFrameworkOptions: FrameworkOptions = {};
Expand Down
4 changes: 2 additions & 2 deletions code/lib/cli/src/sandbox-templates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ const benchTemplates = {
skipTemplateStories: true,
testBuild: true,
},
skipTasks: ['e2e-tests-dev', 'test-runner', 'test-runner-dev', 'e2e-tests', 'chromatic'],
skipTasks: ['e2e-tests-dev', 'test-runner', 'test-runner-dev', 'e2e-tests'],
},
'bench/react-webpack-18-ts-test-build': {
...baseTemplates['react-webpack/18-ts'],
Expand All @@ -578,7 +578,7 @@ const benchTemplates = {
skipTemplateStories: true,
testBuild: true,
},
skipTasks: ['e2e-tests-dev', 'test-runner', 'test-runner-dev', 'e2e-tests', 'chromatic'],
skipTasks: ['e2e-tests-dev', 'test-runner', 'test-runner-dev', 'e2e-tests'],
},
} satisfies Record<string, Template & { isInternal: true }>;

Expand Down
14 changes: 14 additions & 0 deletions 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,6 +185,10 @@ export const previewAnnotations = async (base: any, options: Options) => {
return [...config, ...base];
};

const testBuildFeatures = (value: boolean) => ({
emptyBlocks: value,
});

export const features = async (
existing: StorybookConfig['features']
): Promise<StorybookConfig['features']> => ({
Expand All @@ -196,6 +200,16 @@ export const features = async (
legacyDecoratorFileOrder: false,
});

export const build = async (value: StorybookConfig['build'], options: Options) => {
return {
...value,
test: {
...testBuildFeatures(!!options.test),
...value?.test,
},
};
};

export const csfIndexer: Indexer = {
test: /(stories|story)\.(m?js|ts)x?$/,
createIndex: async (fileName, options) => (await readCsf(fileName, options)).parse().indexInputs,
Expand Down
5 changes: 4 additions & 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,6 @@
// Here we map the name of a module to their NAME in the global scope.
export const globals = {
// eslint-disable-next-line @typescript-eslint/naming-convention,no-underscore-dangle
const _globals = {
'@storybook/addons': '__STORYBOOK_MODULE_ADDONS__',
'@storybook/global': '__STORYBOOK_MODULE_GLOBAL__',
'@storybook/channel-postmessage': '__STORYBOOK_MODULE_CHANNEL_POSTMESSAGE__', // @deprecated: remove in 8.0
Expand All @@ -13,3 +14,5 @@ export const globals = {
'@storybook/preview-api': '__STORYBOOK_MODULE_PREVIEW_API__',
'@storybook/store': '__STORYBOOK_MODULE_STORE__',
};

export const globals: typeof _globals & Record<string, string> = _globals;
11 changes: 11 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 @@ -76,6 +76,7 @@ export interface Presets {
apply(extension: 'managerEntries', config: [], args?: any): Promise<string[]>;
apply(extension: 'refs', config?: [], args?: any): Promise<unknown>;
apply(extension: 'core', config?: {}, args?: any): Promise<CoreConfig>;
apply(extension: 'build', config?: {}, args?: any): Promise<StorybookConfig['build']>;
apply<T>(extension: string, config?: T, args?: unknown): Promise<T>;
}

Expand Down Expand Up @@ -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 •
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 @@ -312,6 +314,15 @@ export interface StorybookConfig {
legacyDecoratorFileOrder?: boolean;
};

build?: {
test?: {
/**
* Globalize @storybook/blocks
*/
emptyBlocks?: boolean;
};
};

/**
* Tells Storybook where to find stories.
*
Expand Down