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

Upgrading to react-scripts@5 #441

Open
tuliomir opened this issue Nov 22, 2023 · 2 comments
Open

Upgrading to react-scripts@5 #441

tuliomir opened this issue Nov 22, 2023 · 2 comments
Assignees
Labels
dependencies Pull requests that update a dependency file

Comments

@tuliomir
Copy link
Collaborator

tuliomir commented Nov 22, 2023

Summary

react-scripts is one of the dependencies contained with the create-react-app bundle that was used to create this application.

According to the [email protected] release announcement, a breaking change was added in that the WebPack version being used is now 5.0. This means that the NodeJs globals such as fs and crypto are now inacessible.

So, as the current state of the code, it is unable to run after an upgrade to react-scripts v5. This also means that running the application on Node versions 17+ require the --openssl-legacy-provider flag as a parameter.

The react-scripts are being updated to version 4 as of now ( PR #426 ), and future dependency upgrades are discussed below. The suggested upgrade path would be:

  • Staying in v4 for the short term
  • Upgrading to v5 on the mid term ( solution 3 )
  • Migrating away from CRA on the long term ( solution 4 ), if the detailed conditions are maintained

Note

A workaround specific to CRA v4 was implemented on #510 that should be reverted when an upgrade to CRA v5 is made, through any of the solutions below.

Solution 1: Polyfill and keep code

It would be possible to upgrade to WebPack v5 and add polyfills if we used it directly.

However, our configuration of WebPack is entirely done through the react scripts, and the config file is not accessible to the developer directly. To fix this, a solution would be to add each polyfill as a separate dependency and integrate them through another dependency such as react-app-rewired.

This adds multiple dependencies to the application and requires the entire react-scripts to be executed in the context of the rewired configuration, which adds security concerns to the application at runtime. See below examples of this implementation:

Solution 2: Fork react-scripts

This is actually a solution common enough to have its own entry on the official docs ( see Alternatives to Ejecting ).

This, however, would require our own maintenance of a complex third-party library and does not seem reasonable for our usage.

Solution 3: Refactor to a web-based application

This solution is the most recommended given enough development time: adapt the application to avoid using all NodeJS modules and move entirely to a browser-based environment.

To do so, the first step would be to upgrade react-scripts to version 4 ( which is being done on #426 ) and then change every dependency to a version that is dedicated to a browser environment.

As this change is complete and the application is entirely a browser one, upgrade the react-scripts. This would be the best course of action for the mid term.

Solution 4: Move to a new framework

As discussed on this video, create-react-app is a heavily opinionated framework designed to improve the learning curve to use react, back when many of today's modern standards for javascript and React development were not in place.

As these standards evolved and drifted away from the proposal of those opinions, the CRA itself starts to become more of a drag: increased maintenance efforts can lead to less trustable or obsolete code bases on our dependencies. Through this point of view, the CRA should be considered a legacy project, and future improvements should consider moving away from it towards newer frameworks.

In this video from June/2022, the ones mentioned were mostly Vite and NextJS, with Vite obtaining more control over the build phase being a slight advantage for an Electron application.

However, this would be a great effort and a project in itself. So, this would be recommended only for a long term project.

@tuliomir tuliomir added the dependencies Pull requests that update a dependency file label Nov 22, 2023
@tuliomir tuliomir self-assigned this Nov 22, 2023
@tuliomir tuliomir moved this from Todo to In Progress (Done) in Hathor Network Nov 22, 2023
@pedroferreira1
Copy link
Member

It makes sense for me to skip this for now and we can discuss this migration in another dependency upgrade project.

@pedroferreira1 pedroferreira1 moved this from In Progress (Done) to In Review (WIP) in Hathor Network Nov 29, 2023
@r4mmer
Copy link
Member

r4mmer commented Dec 4, 2023

We need the nodejs libs mostly for the wallet-lib, but this can change if the lib builds for browsers.
I think a good idea would be configuring the lib to compile 2 separate sets of files, one for browsers and another for nodejs, then we could use the browser tag so that any package that imports our lib will receive a usable version of the lib.

@tuliomir tuliomir moved this from In Review (WIP) to Done in Hathor Network Dec 4, 2023
This was referenced Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
Archived in project
Development

No branches or pull requests

3 participants