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

WebpackBuilder: Remove need for react as peerDependency #23496

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jul 18, 2023

What I did

I noticed that ui/components and lib/manager-api have -rightfully- a peerDependency on react.

Those packages are imported into the webpack builder causing it to also depend on react (via a peerDependency)

That causes the webpack frameworks to all depend on react (via a peerDependency).

The reason why the ui/components and lib/manager-api are used in the webpack builder is unclear:
It's likely a left-over from the olden days when the builder was used for both, and it was just always left in place in some form... never re-evaluated properly.

I left the code in place to add aliases, though I think this code should be removed.
By removing the dependencies the require.resolve used to add the alias will likely start failing in some sitations.
I wrapped it with a try-catch, so if the resolve fails, the alias is not added.

I suspect the alias might only be useful for us internally, not for outside users.

The only need you'd ever have to ensure a single version of the packages (which is what aliases are used for here) would be for addon-docs.
Addon docs should add these aliases if they are needed for some reason IMHO. Addon-docs already has a react dep, so there it would be fine.

How to test

Webpack based sandboxes should continue to work, including docs pages and MDX pages.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

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.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

@ndelangen ndelangen requested a review from shilman July 18, 2023 19:09
@ndelangen
Copy link
Member Author

@shilman 1 of my learnings from my spike, that I deemed safe enough to extract and possible push for 7.2

I'm open to your ideas & feedback

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

HOORAY!!!! 🚀

@ndelangen ndelangen merged commit 80754e3 into next Aug 7, 2023
@ndelangen ndelangen deleted the norbert/react-peerdep-webpack-builder-removal branch August 7, 2023 11:41
@github-actions github-actions bot mentioned this pull request Aug 7, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants