-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Changes from 10 commits
1d5e9f8
9940766
7246a17
dd3ba90
686867f
5fa9f5b
0d52b80
6a6e4cf
cea2827
899ba75
ef36c36
463d488
da5ee52
70ee50f
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,60 @@ | ||
import { extname } from 'path'; | ||
import resolve from 'resolve'; | ||
|
||
export class ReactDocgenResolveError extends Error { | ||
// the magic string that react-docgen uses to check if a module is ignored | ||
readonly code = 'MODULE_NOT_FOUND'; | ||
|
||
constructor(filename: string) { | ||
super(`'${filename}' was ignored by react-docgen.`); | ||
} | ||
} | ||
|
||
/* The below code was copied from: | ||
* https://github.com/reactjs/react-docgen/blob/df2daa8b6f0af693ecc3c4dc49f2246f60552bcb/packages/react-docgen/src/importer/makeFsImporter.ts#L14-L63 | ||
* because it wasn't exported from the react-docgen package. | ||
*/ | ||
|
||
// These extensions are sorted by priority | ||
// resolve() will check for files in the order these extensions are sorted | ||
export const RESOLVE_EXTENSIONS = ['.js', '.ts', '.tsx', '.mjs', '.cjs', '.mts', '.cts', '.jsx']; | ||
|
||
export function defaultLookupModule(filename: string, basedir: string): string { | ||
const resolveOptions = { | ||
basedir, | ||
extensions: RESOLVE_EXTENSIONS, | ||
// we do not need to check core modules as we cannot import them anyway | ||
includeCoreModules: false, | ||
}; | ||
|
||
try { | ||
return resolve.sync(filename, resolveOptions); | ||
} catch (error) { | ||
const ext = extname(filename); | ||
let newFilename: string; | ||
|
||
// if we try to import a JavaScript file it might be that we are actually pointing to | ||
// a TypeScript file. This can happen in ES modules as TypeScript requires to import other | ||
// TypeScript files with .js extensions | ||
// https://www.typescriptlang.org/docs/handbook/esm-node.html#type-in-packagejson-and-new-extensions | ||
switch (ext) { | ||
case '.js': | ||
case '.mjs': | ||
case '.cjs': | ||
newFilename = `${filename.slice(0, -2)}ts`; | ||
break; | ||
|
||
case '.jsx': | ||
newFilename = `${filename.slice(0, -3)}tsx`; | ||
break; | ||
default: | ||
throw error; | ||
} | ||
|
||
return resolve.sync(newFilename, { | ||
...resolveOptions, | ||
// we already know that there is an extension at this point, so no need to check other extensions | ||
extensions: [], | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { | |
parse, | ||
builtinResolvers as docgenResolver, | ||
builtinHandlers as docgenHandlers, | ||
builtinImporters as docgenImporters, | ||
makeFsImporter, | ||
ERROR_CODES, | ||
utils, | ||
} from 'react-docgen'; | ||
|
@@ -11,6 +11,12 @@ import type { LoaderContext } from 'webpack'; | |
import type { Handler, NodePath, babelTypes as t, Documentation } from 'react-docgen'; | ||
import { logger } from '@storybook/node-logger'; | ||
|
||
import { | ||
RESOLVE_EXTENSIONS, | ||
ReactDocgenResolveError, | ||
defaultLookupModule, | ||
} from '../../../../frameworks/react-vite/src/plugins/docgen-resolver'; | ||
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 there anything we can do to clean things up? cc @valentinpalkovic 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 did this on purpose. The only 2 places in our codebase that currently depend on So in order to extract this somewhere else, I'd need to introduce the dependency to some shared package. 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; If you feel like this is so bad we're better off duplicating the code, we could do that? 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. Although this is kind of ugly (requiring files from the I would rather vote to copy the code from or we just duplicate the code in The third option is that things stay as is, but then 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. @ndelangen Just to clarify. Are you talking about extracting 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 define "correctly accessible by both"? @valentinpalkovic
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. 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.
Ah, yes that's right. I missed that. 🙏 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 duplicated the code. |
||
|
||
const { getNameOrValue, isReactForwardRefCall } = utils; | ||
|
||
const actualNameHandler: Handler = function actualNameHandler(documentation, componentDefinition) { | ||
|
@@ -54,7 +60,6 @@ type DocObj = Documentation & { actualName: string }; | |
|
||
const defaultHandlers = Object.values(docgenHandlers).map((handler) => handler); | ||
const defaultResolver = new docgenResolver.FindExportedDefinitionsResolver(); | ||
const defaultImporter = docgenImporters.fsImporter; | ||
const handlers = [...defaultHandlers, actualNameHandler]; | ||
|
||
export default async function reactDocgenLoader( | ||
|
@@ -71,7 +76,15 @@ export default async function reactDocgenLoader( | |
filename: this.resourcePath, | ||
resolver: defaultResolver, | ||
handlers, | ||
importer: defaultImporter, | ||
importer: makeFsImporter((filename, basedir) => { | ||
const result = defaultLookupModule(filename, basedir); | ||
|
||
if (RESOLVE_EXTENSIONS.find((ext) => result.endsWith(ext)) === undefined) { | ||
return result; | ||
} | ||
|
||
throw new ReactDocgenResolveError(filename); | ||
}), | ||
babelOptions: { | ||
babelrc: false, | ||
configFile: false, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.foo { | ||
color: red; | ||
} | ||
ndelangen marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
@shilman here i'm now using the array, do you think this is preferable?
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.
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
storybook/code/frameworks/react-vite/src/plugins/react-docgen.ts
Lines 32 to 56 in 899ba75
So in my defense, I thought that to keep those patterns in sync was pretty reasonable.
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.
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?