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

feat(react): remove unnecessary dependencies from @nrwl/react #13525

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

jaysoo
Copy link
Member

@jaysoo jaysoo commented Nov 30, 2022

This PR removes packages such as @nrwl/webpack and @nrwl/cypress as dependencies from @nrwl/react. The goal is to avoid installing things that are not needed. For example, if vite is used then webpack should not be installed. Same thing for vitest replacing jest.

Technical details

To make sure we can still use other generators inside any generator (such as webpackInitGenerator from @nrwl/webpack), a new utility function is added to @nrwl/devkit: ensurePackage.

This function takes a package and an expected version, and checks to make sure it is already installed in the workspace.

export async function ensurePackage(
  tree: Tree,
  pkg: string,
  requiredVersion: string,
  options: {
    cwd?: string;
    dev?: boolean;
    packageJsonPath?: string;
    packageManager?: 'npm' | 'yarn' | 'pnpm';
    throwOnMissing?: boolean;
  } = {}
): Promise<void> {
  //...
}

If the package is not found, then one of two things happens:

  1. If user passed --dryRun, then throwOnMissing is set to true and an error is thrown with a message like Required package <pkg>@<version> is missing. Run npm install -D <pkg>@<version>, and then try again.
  2. If user did not pass --dryRun then the missing package is installed before continuing with the rest of the invoking generator. This will be the common case, for example when running create-nx-workspace.

Results / Comparisons

A normal React+Vite workspace using --preset=react-standalone will result in 365MB of node_modules prior to this change. After this change we end up with 259MB of node_modules, or a 106MB reduction in size.

A plain vite project created via npm create vite (select React and TypeScript), then adding the following packages:

npm i prettier \
react-test-renderer \
vitest \
@vitest/ui \
@vitejs/plugin-react \
@types/node \
@testing-library/react \
vite-tsconfig-paths

In that plain vite setup, node_modules is 151MB in size. There are still some improvements we can make (such as removing rxjs), but that is out of scope for this PR.

If you add the eslint plugins, then the vite project goes to 208MB in size:

npm i eslint \
eslint-config-prettier \
eslint-plugin-import \
eslint-plugin-jsx-a11y \
eslint-plugin-react \
eslint-plugin-react-hooks \
eslint-config-prettier \
eslint-plugin-import \
eslint-plugin-jsx-a11y \
eslint-plugin-react \
eslint-plugin-react-hooks \
vite-plugin-eslint \
@typescript-eslint/eslint-plugin \
@typescript-eslint/parser

Note: There seems to be duplicated packages at different versions such as aria-query between eslint plugins and testing library.

If you bring in the nx-specific packages, that's another 39MB for a total 247MB (or a 12 MB difference from our react-standalone preset):

 6.0M	node_modules/nrwl
 4.2M	node_modules/nx
 12M	node_modules/@parcel
 17M	node_modules/rxjs

Other notes

  • @nrwl/react now relies fully on vite/webpack/jest/cypress init generators to install the necessary packages.
  • Added uiFramework option to webpack init generator, same option already supported in cypress and storybook.
  • The AsyncIterable helpers from @nrwl/js have been moved to @nrwl/devkit so we can remove the JS dependency from @nrwl/react.

@nx-cloud-staging
Copy link

nx-cloud-staging bot commented Nov 30, 2022

Nx Cloud Report

CI is running for commit 7555360.

📂 Click to track the progress, see the status, the terminal output, and the build insights.


Sent with 💌 from NxCloud.

@vercel
Copy link

vercel bot commented Nov 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nx-dev ✅ Ready (Inspect) Visit Preview Dec 1, 2022 at 9:18PM (UTC)

@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from c92f1db to ca12017 Compare November 30, 2022 23:49
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from ca12017 to 7a2a9b7 Compare December 1, 2022 01:52
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from 7a2a9b7 to 8c18195 Compare December 1, 2022 02:06
@jaysoo jaysoo requested a review from vsavkin December 1, 2022 02:33
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from 8c18195 to b59f9c1 Compare December 1, 2022 04:46
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from b59f9c1 to caf1afa Compare December 1, 2022 13:37
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from caf1afa to 6943ee0 Compare December 1, 2022 13:50
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from 6943ee0 to 40fec3e Compare December 1, 2022 13:59
@jaysoo jaysoo force-pushed the feat/cleanup-react-deps branch from 40fec3e to 6d9c86d Compare December 1, 2022 14:13
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
docs/generated/devkit/index.md Outdated Show resolved Hide resolved
lerna.json Outdated Show resolved Hide resolved
packages/devkit/index.ts Outdated Show resolved Hide resolved
packages/esbuild/src/executors/esbuild/esbuild.impl.ts Outdated Show resolved Hide resolved
- If Vite is used, then Webpack and Jest are not needed
- If e2e is not used, then Cypress is not needed
- Trims down on the amount of packages that are downloaded
@nx-cloud
Copy link

nx-cloud bot commented Dec 1, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 53813a2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 12 targets

Sent with 💌 from NxCloud.

@jaysoo jaysoo enabled auto-merge (squash) December 1, 2022 21:49
@vsavkin vsavkin disabled auto-merge December 1, 2022 21:51
@vsavkin vsavkin enabled auto-merge (rebase) December 1, 2022 21:51
@vsavkin vsavkin disabled auto-merge December 1, 2022 21:54
@@ -18,6 +18,12 @@
"description": "Init Webpack Plugin.",
"type": "object",
"properties": {
"uiFramework": {
"type": "string",
"description": "UI Framework to use for Vite.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. Please fix this in a follow up PR.

import { tsquery } from '@phenomnomnominal/tsquery';

export default async function eslint8Updates(tree: Tree) {
try {
const { addPropertyToJestConfig } = await import('@nrwl/jest');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this have ensurePackage?


/**
* Don't run actual child_process implementation of installPackagesTask()
*/
jest.mock('child_process');
// jest.mock('child_process');
Copy link
Collaborator

Choose a reason for hiding this comment

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

rm

@@ -66,22 +67,54 @@ function updateDependencies(host: Tree, schema: InitSchema) {
});
}

function initRootBabelConfig(tree: Tree, schema: InitSchema) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened here? @mandarini just made a shared one in @nrwl/js

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see it in master, can be addresses later.

{
'@pmmmwh/react-refresh-webpack-plugin':
reactRefreshWebpackPluginVersion,
'@phenomnomnominal/tsquery': tsQueryVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this related to webpack?


// React apps
export const reactRefreshWebpackPluginVersion = '^0.5.7';
export const tsQueryVersion = '4.1.1';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this related to webpack?

@virtuoushub
Copy link
Contributor

virtuoushub commented Dec 7, 2022

I am seeing some odd behavior in 15.3.0 and I am pretty sure it is related to these changes.

after upgrading, I needed to add the following modules in order to get npx nx affected --target=test --parallel=3 --ci --code-coverage and npx nx affected --target=build --parallel=3 to work like in 15.2.4:

  • @babel/preset-react
  • @pmmmwh/react-refresh-webpack-plugin
  • @svgr/webpack
  • url-loader
  • ( maybe react-refresh ? )

I will file a bug report later; just wanted to give a quick heads up in case it helps.

@ViktorPontinen
Copy link
Contributor

I am seeing some odd behavior in 15.3.0 and I am pretty sure it is related to these changes.

after upgrading, I needed to add the following modules in order to get npx nx affected --target=test --parallel=3 --ci --code-coverage and npx nx affected --target=build --parallel=3 to work like in 15.2.4:

  • @babel/preset-react
  • @pmmmwh/react-refresh-webpack-plugin
  • @svgr/webpack
  • url-loader
  • ( maybe react-refresh ? )

I will file a bug report later; just wanted to give a quick heads up in case it helps.

Also noticed issues and had to add react-refresh as a separate dependency.
Not sure how common our setup is tho so might not affect many.
We run react-scripts (CRA(v5)) via craco (v7) using nx:run-commands with cwd inside each application.

@enchorb
Copy link

enchorb commented Dec 8, 2022

Same issue here had to yarn add -D react-refresh & yarn add -D @pmmmwh/react-refresh-webpack-plugin. @FrozenPandaz will this be needed going forward, or a regression that will be reverted in a later version?

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants