Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ReactVite: Docgen ignore un-parsable files #26254
Changes from 9 commits
1d5e9f8
9940766
7246a17
dd3ba90
686867f
5fa9f5b
0d52b80
6a6e4cf
cea2827
899ba75
ef36c36
463d488
da5ee52
70ee50f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 there anything we can do to clean things up? cc @valentinpalkovic
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 did this on purpose.
The only 2 places in our codebase that currently depend on
react-docgen
are: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: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 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 thereact-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 frameworkreact-vite
and the presetreact-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 thereact-webpack
package.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.
@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 anyreact-docgen
dependencies or am i overlooking something?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.
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 suggesteddocs-tools
.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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I duplicated the code.