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 to react-scripts@5.0.0 #712

Closed
aeolianeth opened this issue Mar 21, 2022 · 12 comments
Closed

Upgrade to react-scripts@5.0.0 #712

aeolianeth opened this issue Mar 21, 2022 · 12 comments
Assignees
Labels
Core Core platform function requirements.

Comments

@aeolianeth
Copy link
Contributor

Description

we're currently on react-scripts@4.0.3.

Proposal

Upgrade to react-scripts@5.0.0, to take advantage of bundle size optimizations and faster compile times offered by Webpack 5 (vs Webpack 4).

@aeolianeth aeolianeth added housekeeping Core Core platform function requirements. labels Mar 21, 2022
@sticklo
Copy link
Contributor

sticklo commented Apr 8, 2022

kindly assign this one to me boss @aeolianeth

@aeolianeth
Copy link
Contributor Author

kindly assign this one to me boss @aeolianeth

@sticklo I think you should be able to assign yourself now!

@aeolianeth
Copy link
Contributor Author

@sticklo thanks for looking into this. I hope you don't mind but I'm going to reassign this issue to @benschac. I think you decided to move on from this issue from our Discord conversation anyway 🙏

@aeolianeth aeolianeth assigned benschac and unassigned sticklo Apr 24, 2022
@sticklo
Copy link
Contributor

sticklo commented Apr 24, 2022

Its alright, thank you for the update @aeolianeth.

@benschac
Copy link
Contributor

benschac commented Apr 24, 2022

There isn't a straightforward migration path that I feel comfortable taking without input from the core team that is beyond the scope of this task. (cc: @aeolianeth, @johnnyd-eth, @peripheralist).

facebook/create-react-app#11756

react-scripts@5 uses webpack@5, which no longer ships with polyfills by default. This is problematic because CRA doesn't give us access to the webpack config by default.

After reading through this issue we have a couple of options that are not great.

  1. Eject from CRA, and update the webpack.config.js with polyfills.
  2. Use something like rewired but
As of Create React App 2.0 this repo is "lightly" maintained mostly by the community at this point.

⚠️ Please Note:

By doing this you're breaking the https://github.com/facebookincubator/create-react-app/issues/99#issuecomment-234657710 that CRA provides. That is to say you now "own" the configs. No support will be provided. Proceed with caution.
"Stuff can break" — Dan Abramov https://twitter.com/dan_abramov/status/1045809734069170176
  1. Wait for CRA to address the polyfill issue and continue to use our current setup.
  2. Look into other solutions like vite.

@peripheralist
Copy link
Contributor

I'd say 1 and 2 are off the table. Have you looked into what progress is being made on 3 @benschac ?

I'm sure lots of apps have this problem, I'd be happy to wait as long as we need to for a safe solution with more community adoption

@aeolianeth
Copy link
Contributor Author

Agree, don't want to eject.

What do we need the polyfills for? And what are the missing polyfills? Don't need a specific list yet, just want to get a sense of it

@aeolianeth
Copy link
Contributor Author

We shouldn't need to be polyfilling anything ideally I think - we're only supporting the latest browsers.

I imagine it's Antd or some other dependency that relies on polyfills. If so we could consider upgrading those first.

@benschac
Copy link
Contributor

benschac commented Apr 25, 2022

Progress

There is an open PR (facebook/create-react-app#11764), and it's been marked for release in 5.0.2 The current release is 5.0.1

The PR linked above looks promising after reading the docs a bit (https://github.com/facebook/create-react-app/pull/11764/files#diff-d532c364ffff91db2d2bedd34a7a44d2574fee0dd832ecb9324342704e1eeb85). It mostly looks like minor documentation nitpicks that is holding it up.

Another solution could be to:

A lot of apps do have this problem! Gnosis Safe went with option #2 (safe-global/safe-react-apps#409) via Chain Safe's web3.js documentation (https://github.com/ChainSafe/web3.js#web3-and-create-react-app).

Polyfills

A bit more context on why polyfills are removed in Webpack's documentation (https://webpack.js.org/blog/2020-10-10-webpack-5-release/#automatic-nodejs-polyfills-removed)

What do we need the polyfills for?

This is the post MDN used to define a polyfill.
https://remysharp.com/2010/10/08/what-is-a-polyfill

 A polyfill, or polyfiller, is a piece of code (or plugin) that provides the technology that you, the developer, expect the browser to provide natively. Flattening the API landscape, if you will.

And what are the missing polyfill

There are many! I didn't get them all. After reading through the docs/PRs/Issues, I figured the next best step was getting feedback from the team on next steps.

I imagine it's Antd or some other dependency that relies on polyfills. If so we could consider upgrading those first.

Antd was one of them for sure. We should upgrade it regardless. I'd assume if ChainSafe needs polyfills and has formally documented it, JB will most likely end up needing them too.

@benschac
Copy link
Contributor

benschac commented Apr 25, 2022

My 2 cents: Option 2 seems like a viable path forward after looking at ChainSafe's Docs docs forweb3.js.

@aeolianeth
Copy link
Contributor Author

Closing due to NextJS migration

@aeolianeth aeolianeth moved this from In review to Done in juicebox.money Jun 3, 2022
@benschac
Copy link
Contributor

benschac commented Jun 3, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Core platform function requirements.
Projects
Status: 🧃 Done
Development

No branches or pull requests

4 participants