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

Upgrade CRACO to v7.1.0 & react-scripts to v5.0.1 #2376

Merged
merged 14 commits into from
Mar 23, 2023

Conversation

ianyong
Copy link
Member

@ianyong ianyong commented Mar 12, 2023

Description

We are upgrading react-scripts to v5 so that it can be used with Node.js 18. This is because Node.js 16 is reaching end of life this September.

Changes:

  • Updated CRACO from v6.4.5 to v7.1.0.
  • Updated react-scripts from v4.0.3 to v5.0.1.
    • This also updates Webpack to v5 which is the cause for many of the changes below...
  • Removed selective dependency resolution of react-error-overlay.
  • Removed duplicate ESLint installation.
    • We need to use react-script's version to prevent conflicts in configuration.
  • Enabled syncWebAssembly option for Webpack 5.
  • This is essentially reverting to the behaviour of Webpack 4.
  • Regenerated dependency tree (i.e., deleted yarn.lock and regenerated it).
    • Upgrading react-scripts caused a lot of changes in dependencies. Some of these dependencies need to be hoisted to the top-level of the dependency tree to work, but were not.
    • An undesirable side effect is that the minor versions of other dependencies might have been bumped while doing the regeneration. However, everything still seems to work.
  • Forced @types/react to use v17 instead of v18.
  • Polyfill several Node.js core modules.
    • BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default. This is no longer the case. Verify if you need this module and configure a polyfill for it.

    • Added these polyfills as dev dependencies since they are only needed for Webpack to compile.
  • Ignore missing source map warnings for dependencies.
    • Some of our dependencies do not ship with source maps. There's not much we can do about this, so we just disable the warnings if it comes from the node_modules directory.
  • Specify the process and buffer polyfills as plugins.
    • process is needed for environment variables to work as if they were being used in a Node.js environment (i.e., all of the process.env.ENV_VAR that are being used in the codebase).
    • buffer is needed for some of our dependencies.

Resolves #2313. Fully fixes #2213.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

There should be no change in behaviour.

@ianyong ianyong requested a review from chownces March 12, 2023 11:48
@@ -1,3 +1,6 @@
// eslint-disable-next-line @typescript-eslint/no-var-requires
Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot use ES6 imports for config files.

'os': require.resolve('os-browserify/browser'),
'stream': require.resolve('stream-browserify'),
'timers': require.resolve('timers-browserify'),
'url': require.resolve('url/')
Copy link
Member Author

Choose a reason for hiding this comment

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

The trailing slash was part of the instructions given by Webpack. I'm not very sure if removing it will cause any errors.

@ianyong ianyong requested a review from shenyih0ng March 12, 2023 11:56
@coveralls
Copy link

coveralls commented Mar 12, 2023

Pull Request Test Coverage Report for Build 4496740579

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 34.789%

Totals Coverage Status
Change from base Build 4486105111: 0.0%
Covered Lines: 4668
Relevant Lines: 12504

💛 - Coveralls

@ianyong ianyong force-pushed the upgrade-craco-and-react-scripts branch from 7ebaf50 to 6982c11 Compare March 23, 2023 03:20
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

Great!

@martin-henz martin-henz merged commit 02a8ce8 into master Mar 23, 2023
@martin-henz martin-henz deleted the upgrade-craco-and-react-scripts branch March 23, 2023 03:50
RichDom2185 pushed a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Jul 15, 2023
* Update CRACO from v6.4.5 to v7.1.0

* Update react-scripts from v4.0.3 to v5.0.1

* Remove selective dependency resolution of react-error-overlay

* Remove duplicate ESLint installation - favour CRA's

* Enable old WebAssembly in Webpack 5

* Regenerate dependency tree

To resolve the following error:
Loading PostCSS "postcss-normalize" plugin failed: Cannot find
module 'postcss-normalize'

It seems that some dependencies which should be hoisted to the top-
level of the dependency tree were previously not.

* Force use of React 17 types instead of 18

* Polyfill Node.js core modules

Webpack 5 no longer automatically polyfills many of the Node.js
modules. As such, we need to pull them in ourselves.

* Ignore missing source map warnings for dependencies

* Polyfill 'process' for environment variables to work

* Polyfill 'buffer' for the application to work in the browser

* Move polyfill libraries to dev dependencies

* Update test snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade react-scripts [Development Only] Error thrown & iframe injected when the application is hot reloaded
3 participants