CLI: Add prompt-only automigrate asking for react-removal#25215
CLI: Add prompt-only automigrate asking for react-removal#25215
Conversation
| if (hasMDX) { | ||
| const mdxSuggestion = dedent` | ||
| As you are using '.mdx'-files, it might be reasonable to keep the dependency. | ||
| `; | ||
| return [start, mdxSuggestion].join('\n\n'); | ||
| } |
There was a problem hiding this comment.
The detectMDXEntries is pretty heavy and requires some heavy-lifting file system lookups. I don't think it's worth it just to log this one-liner to the user. What is the worst that can happen? Things might break and the user has to re-add react and react-dom. I think that's acceptable.
There was a problem hiding this comment.
When the user uses .mdx files, they will very likely want to keep the react dependency.
I pulled this code from the override preset, it costs like <80ms. Is that heavy lifting file system lookups to you?
There was a problem hiding this comment.
I don't know in which particular project you did the measurement, but I guess that the automigration might be blocking in bigger projects.
Also, I don't quite understand what you want to achieve:
- You only scan for mdx files, if they are part of the
storiesglob. If they are not defined in the stories glob, you are not checking for them. So why not directly just search formdxin the stories glob pattern? I don't think file system lookups are necessary in this case. - What about MDX usage outside of Storybook. This case isn't covered here, since if the
storiesglob pattern doesn't mentionmdxfiles, they also subsequently will not be found by glob. So if you want to cover this scenario, you don't have to consider thestoriesglob at all but create a simple glob, which considers mdx files and pass it directly to theglobfunction. Also, isn't this case the only relevant one? We want to keepreactandreact-domif e.g. MDX is used outside of Storybook. Isn't this the case?
There was a problem hiding this comment.
I also don't think this check is necessary at all. Early results in #24881 shows that it's perfectly valid to have MDX files - even ones importing React components - in a project without react and react-dom. It just works. TypeScript in the IDE might complain - I haven't tested that out yet - but I don't think that warrants this additional message.
| }, | ||
| prompt({ hasMDX, hasDocs, hasEssentials }) { | ||
| const addons = [hasEssentials ? 'essentials' : '', hasDocs ? 'docs' : ''].filter(Boolean); | ||
| const addonReasonText = |
There was a problem hiding this comment.
This feels like an unnecessary detail to me, I don't think it's needed.
JReinhold
left a comment
JReinhold
left a comment
There was a problem hiding this comment.
I think the special messages for MDX and the addons are unnecessary and we should be able to remove them to simplify the prompt without loosing value.
Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>
Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>
…-react-automigration
…-react-automigration
3 participants
Telescoped: #25196
Works on #25179
What I did
Added an automigration prompt, asking users to remove "react" as a dependency, when it's likely they have it because we asked for it.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake 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/coreteam 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>