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

ReactVite: Docgen ignore un-parsable files #26254

Merged
merged 14 commits into from
Mar 1, 2024

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Feb 29, 2024

Closes: #26109
Closes: #25662

Related: reactjs/react-docgen#892

What I did

I made a video explaining the process:
Watch the video

  • I modified the react-docgen configuration to override the mechanism used to resolve files.
  • When docgen tries to resolve a file, that we don't include in the files to transform, the custom resolver will throw an error.
  • The error will cause react-docgen to skip/ignore the file.
  • Unlike the parsing error, which is fatal and bubbles up to here

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

I performed a manual test by copying over frameworks/react-vite/dist to a reproduction provided by the user (which I modified to make more informative).

The error went away, and docgen information showed up in the bundled assets's code.

The UI changes detected by chromatic are the proof the fix is working.

I added an explicit test to prevent this regression in the future.

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>

@ndelangen ndelangen self-assigned this Feb 29, 2024
@ndelangen ndelangen changed the title filter docgen import resolution files to only JS ReactVite: Docgen ignore un-parsable files Feb 29, 2024
@ndelangen ndelangen removed the request for review from valentinpalkovic February 29, 2024 14:46
@ndelangen
Copy link
Member Author

I'm experimenting with bringing this fix to webpack as well

import {
ReactDocgenResolveError,
defaultLookupModule,
} from '../../../../frameworks/react-vite/src/plugins/docgen-resolver';
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything we can do to clean things up? cc @valentinpalkovic

Copy link
Member Author

@ndelangen ndelangen Feb 29, 2024

Choose a reason for hiding this comment

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

I did this on purpose.

The only 2 places in our codebase that currently depend on react-docgen are:
Screenshot 2024-02-29 at 17 49 37

So in order to extract this somewhere else, I'd need to introduce the dependency to some shared package.
I understand this might look a bit weird, but it's bundled correctly.

I'm open to suggestions that do not create a new package.

Alternatively (I'm not a fan of this idea either) we could add this code to docs-tools;
but then docs tools gains a dependency to react-docgen, and with it, this:
Screenshot 2024-02-29 at 17 53 04

If you feel like this is so bad we're better off duplicating the code, we could do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although this is kind of ugly (requiring files from the react-vite framework into the react-webpack preset things will compile.

I would rather vote to copy the code from frameworks/react-vite/src/plugins/docgen-resolver into a directory that is correctly accessible by both, the framework react-vite and the preset react-webpack, like in e.g. @storybook/core-common

or

we just duplicate the code in ../../../../frameworks/react-vite/src/plugins/docgen-resolver which I think is okay if we don't want to move it into a sharable core package.

The third option is that things stay as is, but then resolve should be a direct dependency of the react-webpack package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ndelangen Just to clarify. Are you talking about extracting code/frameworks/react-vite/src/plugins/docgen-resolver.ts into a core package? Because this file doesn't have any react-docgen dependencies or am i overlooking something?

Copy link
Member Author

@ndelangen ndelangen Feb 29, 2024

Choose a reason for hiding this comment

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

Can you define "correctly accessible by both"? @valentinpalkovic
Placing the code in a shared package, like I mentioned that that would cause that package to gain dependencies it currently does not have.
You suggest core-common, I suggested docs-tools.

Are you talking about extracting code/frameworks/react-vite/src/plugins/docgen-resolver.ts into a core package?

Yes we currently do not have a suitable package, hence I decided to just do this way, which minimizes duplicated code, and optimizes for easy of replaceability because I assume that at some point in the future this code will change upstream and updating it in 2 places going to be fragile, right?

But if you both agree that duplicating the code is better than 1 slightly ugly import, I'll change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolve should be a direct dependency of the react-webpack package.

Ah, yes that's right. I missed that. 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

I duplicated the code.

code/frameworks/react-vite/src/plugins/react-docgen.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@valentinpalkovic valentinpalkovic left a comment

Choose a reason for hiding this comment

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

Thanks for the video! It was very helpful!

code/frameworks/react-vite/src/plugins/react-docgen.ts Outdated Show resolved Hide resolved
import {
ReactDocgenResolveError,
defaultLookupModule,
} from '../../../../frameworks/react-vite/src/plugins/docgen-resolver';
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this is kind of ugly (requiring files from the react-vite framework into the react-webpack preset things will compile.

I would rather vote to copy the code from frameworks/react-vite/src/plugins/docgen-resolver into a directory that is correctly accessible by both, the framework react-vite and the preset react-webpack, like in e.g. @storybook/core-common

or

we just duplicate the code in ../../../../frameworks/react-vite/src/plugins/docgen-resolver which I think is okay if we don't want to move it into a sharable core package.

The third option is that things stay as is, but then resolve should be a direct dependency of the react-webpack package.

@valentinpalkovic
Copy link
Contributor

Great work! 🥳

Comment on lines 54 to 56
if (RESOLVE_EXTENSIONS.find((ext) => result.endsWith(ext)) === undefined) {
return result;
}
Copy link
Member Author

@ndelangen ndelangen Feb 29, 2024

Choose a reason for hiding this comment

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

@shilman here i'm now using the array, do you think this is preferable?

Copy link
Member Author

@ndelangen ndelangen Feb 29, 2024

Choose a reason for hiding this comment

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

I guess we should then also adjust line 29 to support a wider range of file extensions?!
~ @valentinpalkovic

I guess that's done now, but originally I figured we'd keep it the same as the code just slightly above it.
As it made no sense to me to use 2 different patterns.

Check line 33 here (that code has been there like that for 19 months

export function reactDocgen({
include = /\.(mjs|tsx?|jsx?)$/,
exclude = [/node_modules\/.*/],
}: Options = {}): PluginOption {
const cwd = process.cwd();
const filter = createFilter(include, exclude);
return {
name: 'storybook:react-docgen-plugin',
enforce: 'pre',
async transform(src: string, id: string) {
if (!filter(path.relative(cwd, id))) {
return;
}
try {
const docgenResults = parse(src, {
resolver: defaultResolver,
handlers,
importer: makeFsImporter((filename, basedir) => {
const result = defaultLookupModule(filename, basedir);
if (RESOLVE_EXTENSIONS.find((ext) => result.endsWith(ext)) === undefined) {
return result;
}

So in my defense, I thought that to keep those patterns in sync was pretty reasonable.

Copy link
Member Author

@ndelangen ndelangen Feb 29, 2024

Choose a reason for hiding this comment

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

Is the request for me to make a change to the code above (what now line 33)?

AFAIK that code is unrelated to my PR?

@ndelangen ndelangen merged commit 075f911 into next Mar 1, 2024
56 checks passed
@ndelangen ndelangen deleted the norbert/fix-react-docgen-vite-plugin branch March 1, 2024 07:55
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants