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 7 commits into
base: next
Choose a base branch
from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Aug 21, 2024

Closes tobiasdiez/storybook-vue-addon#103

What I did

Currently, the generated imports in storybook-stories.js use the prefix @fs. This prefix is generally used by vite to mark certain imports at the end of the resolution (eg files that outside of the root dir). Adding the prefix already at the beginning yields problems when plugins are modifying the story import resolution (which is the case for the vue plugin).

This is fixed by simplying using the full absolute path of the story (after normalizing Windows paths).

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.5 MB 78.5 MB 0 B 1.99 0%
initSize 151 MB 151 MB 76.8 kB -0.38 0.1%
diffSize 72.6 MB 72.6 MB 76.8 kB -0.61 0.1%
buildSize 6.77 MB 6.77 MB -14 B -0.89 0%
buildSbAddonsSize 1.5 MB 1.5 MB 0 B -0.9 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.83 MB 1.83 MB 0 B -0.88 0%
buildSbPreviewSize 270 kB 270 kB 0 B -0.9 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.8 MB 3.8 MB 0 B -0.89 0%
buildPreviewSize 2.97 MB 2.97 MB -14 B -1.73 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 6s 13.3s 7.2s -0.16 54.7%
generateTime 19.7s 18.7s -1s -44ms -0.75 -5.6%
initTime 15.5s 13.6s -1s -844ms -0.63 -13.5%
buildTime 8.9s 8.9s 30ms -0.78 0.3%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7s 7.9s 840ms 0.58 10.6%
devManagerResponsive 4.6s 5.1s 519ms 0.66 10.1%
devManagerHeaderVisible 554ms 712ms 158ms 0.83 22.2%
devManagerIndexVisible 588ms 750ms 162ms 0.81 21.6%
devStoryVisibleUncached 1.1s 1.4s 328ms 0.74 22%
devStoryVisible 583ms 772ms 189ms 0.98 24.5%
devAutodocsVisible 467ms 658ms 191ms 1.27 🔺29%
devMDXVisible 473ms 574ms 101ms 0.26 17.6%
buildManagerHeaderVisible 529ms 673ms 144ms 0.64 21.4%
buildManagerIndexVisible 542ms 679ms 137ms 0.5 20.2%
buildStoryVisible 567ms 714ms 147ms 0.58 20.6%
buildAutodocsVisible 470ms 671ms 201ms 0.72 30%
buildMDXVisible 509ms 548ms 39ms 0.18 7.1%

Greptile Summary

This PR removes the '@fs' prefix from story imports in the Vite builder for Storybook, addressing issues with plugins modifying story import resolution.

  • Refactored toImportFn in codegen-importfn-script.ts to use absolute paths and improve dynamic imports
  • Updated generateModernIframeScriptCode in codegen-modern-iframe-script.ts for better preview annotation handling and HMR support
  • Simplified processPreviewAnnotation in process-preview-annotation.ts to normalize paths using the 'pathe' library
  • Added new test files codegen-importfn-script.test.ts and codegen-modern-iframe-script.test.ts to ensure correct functionality
  • Updated dependencies in package.json to include 'knitwork', 'pathe', and 'slash' for improved path handling

Copy link

nx-cloud bot commented Aug 21, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1549073. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings


import { generateModernIframeScriptCodeFromPreviews } from './codegen-modern-iframe-script';

const projectRoot = 'projectRoot';
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more descriptive variable name, such as mockProjectRoot

Comment on lines 13 to 15
console.log(
'Deprecated: Preview annotations should be strings, not objects. Use the `absolute` property instead.'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a logger instead of console.log for consistency

@tobiasdiez tobiasdiez added builder-vite ci:daily Run the CI jobs that normally run in the daily job. labels Aug 21, 2024
@tobiasdiez
Copy link
Contributor Author

Tests are passing, so this should be good to go.

@JReinhold
Copy link
Contributor

JReinhold commented Aug 23, 2024

Do these changes relate/interfere with the changes proposed in #26010 ?

@tobiasdiez
Copy link
Contributor Author

Do these changes relate/interfere with the changes proposed in #26010 ?

Apart from the obvious merge conflicts there is no interference. The changes in #26010 related to the story index involve virtual modules (prefixed with virtual:) while this PR here enables to use semi-virtual imports that use query selectors (e.g. Button.stories.ts?story=abc).

@JReinhold JReinhold self-assigned this Aug 26, 2024
@tobiasdiez
Copy link
Contributor Author

@JReinhold could we please get this in? It's blocking support for v8 in the vue plugin.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

I'm very sorry for taking so long to look at this, it slipped between the cracks! 🙇

I'm fine with the changes to remove @fs, but not a fan of the new dependencies as they seem unnecessary to me. But as I say, there might just be some context I'm missing.

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 package.json for a minimal storybook. Why is not one storybook-vue enough (that comes with the cli, the builder, the framework, the essential addons)? But reducing this "bloat" may well mean that you install a few packages by default for users which they might not need and thus actually increase a different kind of "bloat".

The node_modules folder for average-size projects seems be easily above 1GB - then I really don't care about improvements in the order of <10mb (other users might of course see this different).

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

import { genDynamicImport, genImport, genObjectFromRawEntries } from 'knitwork';
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 slash dependency, and the 2 or 3 independent implementations of path normalizing in storybook (https://github.com/search?q=repo%3Astorybookjs%2Fstorybook%20slash&type=code)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean.
So what you're saying is that using pathe everywhere is mentally easier/safer for us than using node:path + slash everywhere?

We've sort of been going in the opposite direction recetly, eg. replacing fs-extra with node:fs and a handful of manual logic, to slim down everything and make use of the modern APIs Node gives us today.

However when prebundled, pathe vs slash is 1.68 KB vs 98 B, which is an abysmal difference TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. So what you're saying is that using pathe everywhere is mentally easier/safer for us than using node:path + slash everywhere?

Yes. Additionally, pathe handles all kind of slashes also as input well (eg when calling resolve etc). So you don't need to worry about converting them to the correct format before calling the methods from path. slash is only this tiny bit of pathe: https://github.com/unjs/pathe/blob/main/src/_internal.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@tobiasdiez
Copy link
Contributor Author

@JReinhold After rereading my messages from yesterday, I would like to say sorry for the tone. Had a bit of a rough day yesterday and then got a bit frustrated with what I perceived are minor points. But, of course, you and the team have more insights into the whole ecosystem than me, and I trust your judgments. So please don't take these comments personal. Sorry!

@JReinhold
Copy link
Contributor

No worries @tobiasdiez, we're all humans and I didn't put too much into it anyway. Thanks for being honest, makes everything easier.

@JReinhold
Copy link
Contributor

I suggest you slim down this PR to the minimal changes required to get rid of the @fs prefix. The discussions about dependencies and code logic are becoming bigger than the main change itself, so they should be had in separate PRs that specifically introduces those changes.

@tobiasdiez
Copy link
Contributor Author

I suggest you slim down this PR to the minimal changes required to get rid of the @fs prefix. The discussions about dependencies and code logic are becoming bigger than the main change itself, so they should be had in separate PRs that specifically introduces those changes.

I cannot really slim this down further without losing confidence that it still works. There are a lot of edge cases to consider (input as relative or absolute, in unix or windows style, with ending slash and without, etc). The solution using knitwork + pathe is taken from the nuxt project, that use them in exactly this way to generate dynamic imports for a very similar use case (dynamic imports of "modules"). So I regard this as well tested, also with strange inputs.

If there were a lot of unit tests for this part of storybook, I would be more confident in eg. replacing pathe by the simple slash replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-vite ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Do not know how to load path" error in Storybook 8
2 participants