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

Vite: Don't prefix story import with @fs #28941

Open
wants to merge 6 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion code/builders/builder-vite/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,16 @@
"express": "^4.19.2",
"find-cache-dir": "^3.0.0",
"fs-extra": "^11.1.0",
"knitwork": "^1.1.0",
"magic-string": "^0.30.0",
"pathe": "^1.1.2",
"slash": "^5.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why move slash to a dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually used in a few instances in the builder, eg

So I thought it would be better be listed as a proper dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point of it being a devDependency is so that it gets bundled in (and minimized, optimized, tree-shaken) automatically when we build the package for publication.
Ideally we want to do this with all our dependencies, but some are incompatible with this and some pacakges we just haven't gotten around to test if they can be bundled in, which is why some are still regular dependencies. But devDependencies is the default unless it's not possible.

I understand how that is not obvious from your point of view, because that's usually not how projects use dependencies and devDependencies. We might someday move to a more explicit version of this pattern.

"ts-dedent": "^2.0.0"
},
"devDependencies": {
"@types/express": "^4.17.21",
"@types/node": "^22.0.0",
"glob": "^10.0.0",
"slash": "^5.0.0",
"typescript": "^5.3.2",
"vite": "^4.0.4"
},
Expand Down
56 changes: 56 additions & 0 deletions code/builders/builder-vite/src/codegen-importfn-script.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';

import { toImportFn } from './codegen-importfn-script';

describe('toImportFn', () => {
it('should correctly map story paths to import functions for absolute paths on Linux', async () => {
const root = '/absolute/path';
const stories = ['/absolute/path/to/story1.js', '/absolute/path/to/story2.js'];

const result = await toImportFn(root, stories);

expect(result).toMatchInlineSnapshot(`
"const importers = {
"./to/story1.js": () => import("/absolute/path/to/story1.js"),
"./to/story2.js": () => import("/absolute/path/to/story2.js")
};
export async function importFn(path) {
return await importers[path]();
}"
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
`);
});

it('should correctly map story paths to import functions for absolute paths on Windows', async () => {
const root = 'C:\\absolute\\path';
const stories = ['C:\\absolute\\path\\to\\story1.js', 'C:\\absolute\\path\\to\\story2.js'];

const result = await toImportFn(root, stories);

expect(result).toMatchInlineSnapshot(`
"const importers = {
"./to/story1.js": () => import("C:/absolute/path/to/story1.js"),
"./to/story2.js": () => import("C:/absolute/path/to/story2.js")
};
export async function importFn(path) {
return await importers[path]();
}"
`);
});

it('should handle an empty array of stories', async () => {
const root = '/absolute/path';
const stories: string[] = [];

const result = await toImportFn(root, stories);

expect(result).toMatchInlineSnapshot(`
"const importers = {};
export async function importFn(path) {
return await importers[path]();
}"
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
`);
});
});
31 changes: 16 additions & 15 deletions code/builders/builder-vite/src/codegen-importfn-script.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { relative } from 'node:path';

import type { Options } from 'storybook/internal/types';

import { genDynamicImport, genImport, genObjectFromRawEntries } from 'knitwork';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of using this package for multiple reasons:

  1. With our recent focus on slimming down Storybook, we're trying to ensure that every KB counts, and to me it doesn't look like this improves the code in any way. The library covers a lot of edge cases/options which I don't think is relevant to us - but maybe I'm missing something?
  2. I feel like the readability suffers here, because now to understand what the code does, you need to dig into what knitwork is and does. Before it was just simple string concatenations, which you could understand by understanding basic JS.

But again, maybe this adds compatibility for edge cases that users are experiencing which I don't know about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This library just makes it easier to create safe javascript. Under the hood it's also only string concatenation. To address your points:

With our recent focus on slimming down Storybook, we're trying to ensure that every KB counts

The package size of the builder should be the same (or actually slightly goes down since its shorter to write the code using knitwork over manual string concats). I also don't quite understand why the builder size would matter much, since it is never deployed to any server.

I feel like the readability suffers here

I would argue this is very subjective. Personally, I find genDynamicImport(path) more readable than

`async () => import('${path})`

and it is less prone to make a small mistake (i.e. don't close ' as I did above).

adds compatibility for edge cases

It mostly adds proper handling of special characters in the import path similar to https://github.com/rollup/rollup/blob/master/src/utils/escapeId.ts.

In #28798 I also use it to generate a correct variable name based on a string, which is also somewhat non-trivial.

Copy link
Contributor

@JReinhold JReinhold Sep 11, 2024

Choose a reason for hiding this comment

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

The package size of the builder should be the same (or actually slightly goes down since its shorter to write the code using knitwork over manual string concats).

From a bundled perspective yes, but not when you take into account that the extra dependencies need to be downloaded and installed.

I also don't quite understand why the builder size would matter much, since it is never deployed to any server.

I'm sorry I didn't make this clear in the original comment, but for us it's not just about the size of the final bundle of a built Storybook, it's about the whole Storybook experience, which includes how fast it installs, how many dependencies it adds to your project, how many MBs it adds to your node_modules.
By far the biggest complaint about Storybook today is that it's "bloated" and people don't want to add the bloat to their projects.
You can very rightfully interpret that complaint however you like, but it doesn't change the fact that our users are loud and clear about caring about this, and so do we.

This is why the team has decided that every new dependency will be scrutinized.

You can read more about the ethos at https://e18e.dev/guide/cleanup.html and our perspective on it in #29038.

I would argue this is very subjective. Personally, I find genDynamicImport(path) more readable

Subjective yes. IMO genDynamicImport is only more readable if you understand what it does, which most don't.

and it is less prone to make a small mistake

I can 100% agree with that.

It doesn't make sense to me to replace `async () => import('${path}')` with a dependency.

import { normalize, relative } from 'pathe';
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to above, how does this improve over the current combination of Node and Vite APIs, to justify the extra dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, there is no easy way to normalize windows paths in Node to always use forward-slash, which is what vite needs to handle the import correctly.

Copy link
Contributor

@JReinhold JReinhold Sep 11, 2024

Choose a reason for hiding this comment

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

But isn't that what we did already with normalizePath from Vite at the old line 30-32 below?

https://vitejs.dev/guide/api-plugin.html#path-normalization

import { dedent } from 'ts-dedent';

import { listStories } from './list-stories';

/**
Expand All @@ -21,34 +23,33 @@ function toImportPath(relativePath: string) {
/**
* This function takes an array of stories and creates a mapping between the stories' relative paths
* to the working directory and their dynamic imports. The import is done in an asynchronous
* function to delay loading. It then creates a function, `importFn(path)`, which resolves a path to
* an import function and this is called by Storybook to fetch a story dynamically when needed.
* function to delay loading and to allow Vite to split the code into smaller chunks. It then
* creates a function, `importFn(path)`, which resolves a path to an import function and this is
* called by Storybook to fetch a story dynamically when needed.
*
* @param stories An array of absolute story paths.
*/
async function toImportFn(stories: string[]) {
const { normalizePath } = await import('vite');
export async function toImportFn(root: string, stories: string[]) {
const objectEntries = stories.map((file) => {
const relativePath = normalizePath(relative(process.cwd(), file));
const relativePath = relative(root, file);

return ` '${toImportPath(relativePath)}': async () => import('/@fs/${file}')`;
return [toImportPath(relativePath), genDynamicImport(normalize(file))] as [string, string];
});

return `
const importers = {
${objectEntries.join(',\n')}
};
return dedent`
const importers = ${genObjectFromRawEntries(objectEntries)};

export async function importFn(path) {
return importers[path]();
return await importers[path]();
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
}
`;
}

export async function generateImportFnScriptCode(options: Options) {
export async function generateImportFnScriptCode(options: Options): Promise<string> {
// First we need to get an array of stories and their absolute paths.
const stories = await listStories(options);

// We can then call toImportFn to create a function that can be used to load each story dynamically.
return (await toImportFn(stories)).trim();
// eslint-disable-next-line @typescript-eslint/return-await
return await toImportFn(options.projectRoot || process.cwd(), stories);
}
6 changes: 3 additions & 3 deletions code/builders/builder-vite/src/vite-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,18 @@ export async function commonConfig(

const { viteConfigPath } = await getBuilderOptions<BuilderOptions>(options);

const projectRoot = resolve(options.configDir, '..');
options.projectRoot = options.projectRoot || resolve(options.configDir, '..');

// I destructure away the `build` property from the user's config object
// I do this because I can contain config that breaks storybook, such as we had in a lit project.
// If the user needs to configure the `build` they need to do so in the viteFinal function in main.js.
const { config: { build: buildProperty = undefined, ...userConfig } = {} } =
(await loadConfigFromFile(configEnv, viteConfigPath, projectRoot)) ?? {};
(await loadConfigFromFile(configEnv, viteConfigPath, options.projectRoot)) ?? {};

const sbConfig: InlineConfig = {
configFile: false,
cacheDir: resolvePathInStorybookCache('sb-vite', options.cacheKey),
root: projectRoot,
root: options.projectRoot,
// Allow storybook deployed as subfolder. See https://github.com/storybookjs/builder-vite/issues/238
base: './',
plugins: await pluginConfig(options),
Expand Down
1 change: 1 addition & 0 deletions code/core/src/types/modules/core-common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ export interface BuilderOptions {
ignorePreview?: boolean;
cache?: FileSystemCache;
configDir: string;
projectRoot?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you elaborate on why this is needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not strictly necessary, but before the paths were relative to cwd (which might not be the project root). I've actually extracted the root directory actually only to make testing of the toImportFn simpler - but then thought that I would make sense to propagate this upwards and add a proper handling of the project root.
https://github.com/storybookjs/storybook/pull/28941/files#diff-a4ade24974a8c7f082261c73f22be07ec54ad61a3f9683b20035955a50fa1a8bR54

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned that this doesn't actually solve the issue, but appears to. I wouldn't be surprised if there were hundreds of other places that assumed that process.cwd() === projectRoot, that would break if anyone attempted to use a project root different from the CWD. So then they'd set this explicitly, thinking that that would solve the issue, when it would just cause breakage somewhere else.

It's just my experience with the code base that makes me slightly defensive about this. We would need to set up an actual minimal reproduction with a case where CWD is not project root and see that it worked, before I'd feel confident in this.

I do like your thinking behind this though, on the surface it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really a user-facing option? That was not my intention. I only set it once to be the parent of the config dir.
But sure, there might be other problems with not using the root as cwd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure about it being user-facing, it's at least readable by users, but I don't think it's writable.

Would still be great to confirm it works.

docsMode?: boolean;
features?: StorybookConfigRaw['features'];
versionCheck?: VersionCheck;
Expand Down
9 changes: 9 additions & 0 deletions code/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5804,7 +5804,9 @@ __metadata:
find-cache-dir: "npm:^3.0.0"
fs-extra: "npm:^11.1.0"
glob: "npm:^10.0.0"
knitwork: "npm:^1.1.0"
magic-string: "npm:^0.30.0"
pathe: "npm:^1.1.2"
slash: "npm:^5.0.0"
ts-dedent: "npm:^2.0.0"
typescript: "npm:^5.3.2"
Expand Down Expand Up @@ -18844,6 +18846,13 @@ __metadata:
languageName: node
linkType: hard

"knitwork@npm:^1.1.0":
version: 1.1.0
resolution: "knitwork@npm:1.1.0"
checksum: 10c0/e23c679d4ded01890ab2669ccde2e85e4d7e6ba327b1395ff657f8067c7d73dc134fc8cd8188c653de4a687be7fa9c130bd36c3e2f76d8685e8b97ff8b30779c
languageName: node
linkType: hard

"language-subtag-registry@npm:^0.3.20":
version: 0.3.22
resolution: "language-subtag-registry@npm:0.3.22"
Expand Down