-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Changes from all commits
e23fef9
c9107a1
e4b01dd
ea33f99
d14d4cc
36636ec
1549073
8936c5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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
|
||
`); | ||
}); | ||
}); |
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'; | ||
import { normalize, relative } from 'pathe'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of using this package for multiple reasons:
But again, maybe this adds compatibility for edge cases that users are experiencing which I don't know about? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 would argue this is very subjective. Personally, I find
and it is less prone to make a small mistake (i.e. don't close
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From a bundled perspective yes, but not when you take into account that the extra dependencies need to be downloaded and installed.
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 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.
Subjective yes. IMO
I can 100% agree with that. It doesn't make sense to me to replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you really think that reducing 30kb in node_modules for developers (not production) is worth the danger of introducing bugs by manual string concatenation, may I ask you to migrate the code away from knitwork (here as well as in #28798)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think I'm in a position to interpret that complaint in any way. But shouldn't you know (and not interpret) what most users mean when they say this? Personally, I would generally agree with the statement, but would associate it more with the fact that I have to install 5 different packages in The |
||
import { dedent } from 'ts-dedent'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But isn't that what we did already with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I wasn't aware of the existence of that helper (at least not the active part of my brain). I still think it would make sense to just use pathe everywhere, because it makes you forget about the issue completely. Would also allow to get rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean. We've sort of been going in the opposite direction recetly, eg. replacing However when prebundled, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Additionally, pathe handles all kind of slashes also as input well (eg when calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just had a look at the vite implementation, and it looks like they also just replace the slash: https://github.com/vitejs/vite/blob/5cac0544dca2764f0114aac38e9922a0c13d7ef4/packages/vite/src/shared/utils.ts#L27-L29 |
||
|
||
import { listStories } from './list-stories'; | ||
|
||
/** | ||
|
@@ -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); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -194,6 +194,7 @@ export interface BuilderOptions { | |
ignorePreview?: boolean; | ||
cache?: FileSystemCache; | ||
configDir: string; | ||
projectRoot?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you elaborate on why this is needed now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not strictly necessary, but before the paths were relative to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"name": "nuxt-vite/default-ts", | ||
"$schema": "../../node_modules/nx/schemas/project-schema.json", | ||
"projectType": "application", | ||
"implicitDependencies": [ | ||
"storybook", | ||
"core", | ||
"addon-essentials", | ||
"addon-interactions", | ||
"addon-links", | ||
"addon-onboarding", | ||
"blocks", | ||
"vue3-vite" | ||
], | ||
"targets": { | ||
"sandbox": {}, | ||
"sb:dev": {}, | ||
"sb:build": {} | ||
}, | ||
"tags": ["ci:normal", "ci:merged", "ci:daily"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why move
slash
to adependency
?There was a problem hiding this comment.
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
storybook/code/builders/builder-vite/src/utils/process-preview-annotation.ts
Line 6 in 66b439e
There was a problem hiding this comment.
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
. ButdevDependencies
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
anddevDependencies
. We might someday move to a more explicit version of this pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will move it back to dev dependency.
May I add that I'm not a big fan of this inline-practice (for the builder and framework - it makes completely sense for the cli and the react-based view). I don't see what dependencies you pull-in (along with their security concerns etc, I have to trust you), it makes package managers obsolete / helpless, it makes auditing & sponsoring of indirect dependencies impossible, and it might well increase the overall size of the node modules folder (definitely if everyone would follow such a practice) - and finally it artificially reduces download numbers for npm packages - and we all know that such numbers might be important to get funding for open source projects etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, those are all valid considerations to include when weighing the trade offs.