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

UpdateToNode18 #789

Merged
merged 4 commits into from
Jun 8, 2024
Merged

UpdateToNode18 #789

merged 4 commits into from
Jun 8, 2024

Conversation

angrave
Copy link
Collaborator

@angrave angrave commented Feb 1, 2024

No description provided.

@harsh183
Copy link
Member

Just hit update to see where the CI is failing. A new node version is desperately needed but we can slowly upgrade to the latest version (v22)

@harsh183 harsh183 self-assigned this May 17, 2024
@harsh183
Copy link
Member

harsh183 commented May 24, 2024

Across a lot of different node versions I tried (18, 19, 20), the solution to the current build failing is to update "react-scripts": "5.0.1". However, this upgrades to a version of webpack that doesn't have many of the core library polyfills: facebook/create-react-app#11756.

Module not found: Error: Can't resolve 'https' in '/Users/harsh183/Experiments/FrontEnd/src/utils/cthttp'
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.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "https": require.resolve("https-browserify") }'
	- install 'https-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "https": false }

To apply the polyfills ourselves we can either 'eject' the create react apps config (one way irreversible) to access the config or override the existing config via craco/react-app-rewired.

@harsh183
Copy link
Member

@angrave After doing some more reading, I think the best option is to eject the create react app config. The app has been stably running for the last few years so we're past the initial prototyping phase where we wanted to hide away the config details.

@angrave
Copy link
Collaborator Author

angrave commented May 26, 2024

@harsh183
((The last merged PR was an improved build action by Rob Kooper - so maybe the Docker build error is due to that instead??)

I googled the error below "Error: error:0308010C:digital envelope routines::unsupported"
https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported

extract below from build,
https://github.com/classtranscribe/FrontEnd/actions/runs/9244786161/job/25430487004?pr=789

[frontend 8/8] RUN yarn build
#7 0.298 yarn run v1.22.19
#7 0.328 $ react-scripts build
#7 2.243 Creating an optimized production build...
#7 2.581 Error: error:0308010C:digital envelope routines::unsupported
#7 2.581     at new Hash (node:internal/crypto/hash:69:19)
#7 2.581     at Object.createHash (node:crypto:133:10)
#7 2.581     at module.exports (/frontend/node_modules/webpack/lib/util/createHash.js:135:53)
#7 2.581     at NormalModule._initBuildHash (/frontend/node_modules/webpack/lib/NormalModule.js:417:16)
#7 2.581     at handleParseError (/frontend/node_modules/webpack/lib/NormalModule.js:471:10)
#7 2.581     at /frontend/node_modules/webpack/lib/NormalModule.js:503:5
#7 2.581     at /frontend/node_modules/webpack/lib/NormalModule.js:358:12
#7 2.581     at /frontend/node_modules/loader-runner/lib/LoaderRunner.js:373:3
#7 2.581     at iterateNormalLoaders (/frontend/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
#7 2.581     at iterateNormalLoaders (/frontend/node_modules/loader-runner/lib/LoaderRunner.js:221:10)
#7 2.581     at /frontend/node_modules/loader-runner/lib/LoaderRunner.js:236:3
#7 2.581     at runSyncOrAsync (/frontend/node_modules/loader-runner/lib/LoaderRunner.js:130:11)
#7 2.581     at iterateNormalLoaders (/frontend/node_modules/loader-runner/lib/LoaderRunner.js:232:2)
#7 2.581     at Array.<anonymous> (/frontend/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
#7 2.581     at Storage.finished (/frontend/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
#7 2.581     at /frontend/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9
#7 2.627 /frontend/node_modules/react-scripts/scripts/build.js:19
#7 2.627   throw err;

@harsh183
Copy link
Member

@angrave yeah I was also getting this error locally. Basically this is caused by an old version of react scripts (0.4) currently and moving up to 0.5 will fix it, but that upgrade is another can of worms with react scripts hiding away the launch config.

We can also just allow unsafe openssl, but that feels like a bad short term solution over just fixing the configuration long term (react scripts eject)

@angrave
Copy link
Collaborator Author

angrave commented May 26, 2024

Okay, I'll ignore this PR for now, but will leave it open so the comments are handy. (I think we probably should have put these comments & discussion in a github issue instead of a PR, ... but whatever). FYI One idea I had was starting from a blank slate (e.g. latest react (5?) project) and slowly re-adding required projects., starting with latest versions .I didn't do this because i) we don't have a good testing framework. ii) dva is obviously a blocking component. Looking at this today, dva seems to be used a small shim around redux, (to make it easy to have scope/namespaces)
.
So maybe updating piecemeal to use modern redux is one way to tear out dva...
https://redux.js.org/usage/migrating-to-modern-redux

@harsh183
Copy link
Member

harsh183 commented Jun 4, 2024

@angrave starting over will definitely take much longer, especially when it's already being used by lots of users currently. I think we can incrementally remove dvaJs as needed. Let's continue this discussion here: #768

@angrave
Copy link
Collaborator Author

angrave commented Jun 4, 2024

Please merge/rebase latest changes in staging and re push.

@harsh183
Copy link
Member

harsh183 commented Jun 8, 2024

@angrave the build is passing 🥳 , I can update the documentation once we merge to now have node 18.

@angrave angrave merged commit 67f212a into staging Jun 8, 2024
2 checks passed
@angrave angrave deleted the Updatetonode18 branch June 8, 2024 00:32
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.

2 participants