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

Fork, or copy&paste used react-dev-utils modules into core #7289

Closed
2 tasks done
Josh-Cena opened this issue May 3, 2022 · 3 comments · Fixed by #10956
Closed
2 tasks done

Fork, or copy&paste used react-dev-utils modules into core #7289

Josh-Cena opened this issue May 3, 2022 · 3 comments · Fixed by #10956
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee domain: dependencies Proposal to upgrade a dependency across major versions proposal This issue is a proposal, usually non-trivial change

Comments

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented May 3, 2022

Have you read the Contributing Guidelines on issues?

Motivation

react-dev-utils is a huge package. I propose to stop including react-dev-utils as a dependency.

What's the problem?

The major problem is that it pulls in too many dependencies, some of which break our own infrastructure. For example, this E2E test is failing because fork-ts-checker-webpack-plugin is missing a peer-dependency. However, do we actually need that plugin? —No. Will we ever need it? —Unlikely. (My philosophy is we should never type-check source files during Webpack build, so I don't like the idea of the plugin from the start.)

Take another example. We also install @types/react-dev-utils. Now this is even more headaches, because it also pulls in @types/webpack and @types/webpack-dev-server. But our own webpack and WDS versions are new enough that they include their own types. The two type sources cause conflicts, which forces us to turn on skipLibCheck, making type-checking our internal packages a lot more complicated. (See #7147)

Do we actually need react-dev-utils as a whole?

No. RDU is made up of many disjoint modules (some 30+ of them), and we only use 4: openBrowser, WebpackDevServerUtils, evalSourceMapMiddleware, formatWebpackMessages. Each of them is about 50~150 lines long, far from being unmaintainable if we choose to fork them.

However, the downside of including react-dev-utils as a whole is that we pull in all those dependencies that we don't need, and their respective types dependencies. This adds unnecessary troubles to maintain our own node_modules, and our users' as well.

That sounds abstract. Do we have data?

Sure.

The dep tree of RDU

image

The dependencies we can get rid of if we remove RDU and its types
  • @types/clean-css
  • @types/html-minifier
  • @types/html-webpack-plugin
  • @types/relateurl
  • @types/source-list-map
  • @types/tapable
  • @types/uglify-js
  • @types/webpack-dev-server (!)
  • @types/webpack-sources
  • @types/webpack (!)
  • cosmiconfig
  • detect-port-alt (this is absurd. We have both detect-port and a fork in node_modules.)
  • filesize
  • fork-ts-checker-webpack-plugin (!)
  • http-proxy-middleware
  • immer (!)
  • is-root (we used to rely on it, but the source code is literally just process.getuid?.() === 0. I don't see why any sane person would use it.)
  • loader-utils
  • pkg-up
  • react-error-overlay
  • recursive-readdir
  • schema-utils
  • shell-quote
  • source-map
  • tapable

Plus some dependencies that are deduplicated because they differ only by minor versions. The lock file would just look much cleaner.

Installed node_modules size when running npx create-docusaurus test-website classic:

  • Before: 242M
  • After: 176M

That's a 27% reduction.

Are there other solutions?

We could also convince the CRA team to publish RDU as separate modules. However, given the amount of traffic on their repo and the unresponsiveness, I would rather not bother :) It's also unlikely they do it, because it's outside their intended usage of this package.

See facebook/create-react-app#7068

Is is good?

AFAICT, CRA doesn't even recommend people to use RDU outside CRA, because it's ultimately relying on the internals and could break unintentionally between versions, if they choose to change the implementation, or delete a module altogether.

I understand there could be some Facebook politics here about "don't copy&paste dependencies into our codebase", but as I've exemplified, doing so is far more beneficial than problems of maintainability. We have precedent: react-dev-utils/getProcessForPort is not imported, because we simply copy&paste it into our own commandUtils. The benefit is that we can change the implementation or refactor it at any time.

It's also stated in their README that people can feel free to fork or copy&paste it at any time, which I take as waiving all copyright problems. We even share the same license, so it shouldn't be problematic at all!

Self-service

  • I'd be willing to do some initial work on this proposal myself.
@Josh-Cena Josh-Cena added proposal This issue is a proposal, usually non-trivial change pr: dependencies Pull requests that update a dependency file domain: dependencies Proposal to upgrade a dependency across major versions and removed pr: dependencies Pull requests that update a dependency file labels May 3, 2022
@slorber slorber added the apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee label Sep 25, 2023
@Danielku15
Copy link

Might be an old topic but I think it might become more and more relevant now. It causes non-maintained dependency tree to be pulled into Docusaurus causing even peer-dependency conflicts upgrading to React 19.

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react
npm WARN   react@"^19.0.0" from the root project
npm WARN   30 more (@docsearch/react, @docusaurus/core, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react@"^16.13.1 || ^17.0.0 || ^18.0.0" from [email protected]
npm WARN node_modules/@docusaurus/plugin-debug/node_modules/react-json-view-lite
npm WARN   react-json-view-lite@"^1.2.0" from @docusaurus/[email protected]
npm WARN   node_modules/@docusaurus/plugin-debug
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN   peer react@"^16.13.1 || ^17.0.0 || ^18.0.0" from [email protected]
npm WARN   node_modules/@docusaurus/plugin-debug/node_modules/react-json-view-lite
npm WARN     react-json-view-lite@"^1.2.0" from @docusaurus/[email protected]
npm WARN     node_modules/@docusaurus/plugin-debug

Should be a very easy and quick fix to copy-paste over the few helper functions.

@slorber
Copy link
Collaborator

slorber commented Feb 28, 2025

I'm working on it right now, but the warnings to show above are not related to react-dev-utils

We fixed react-json-view-lite warnings for the upcoming v3.8 already

@slorber
Copy link
Collaborator

slorber commented Feb 28, 2025

Installed node_modules size when running npx create-docusaurus test-website classic:

  • Before: 242M
  • After: 176M

That's a 27% reduction.

I did the cleanup, and it removed many packages mentioned in your list, but not all though.

Unfortunately, from my tests the size before/after is not as significant. Wonder how you measured that, or maybe the numbers have changed.

#10956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee domain: dependencies Proposal to upgrade a dependency across major versions proposal This issue is a proposal, usually non-trivial change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants