classtranscribe / FrontEnd

The React + Redux Frontend for ClassTranscribe
https://classtranscribe.illinois.edu
Other
25 stars 28 forks source link

Update package.json versions (Node / React / node-sass) #625

Closed angrave closed 5 months ago

angrave commented 1 year ago

We're currently on node 14 and older versions of react, react build scripts etc. We need to update to more modern js packages. Any help in moving any/all packages would be appreciated and most useful.

https://github.com/classtranscribe/FrontEnd/blob/staging/package.json https://github.com/classtranscribe/FrontEnd/blob/staging/yarn.lock

We're build using yarn on node version 14 i.e.

nvm install 14 nvm use 14 npm install --global yarn yarn install yarn build

should be all you need.

Notes so far based on what I tried-

We're currently use node-sass (but should probably move over to sass, which is a pure js solution) which is dependent on the version of node-

https://stackoverflow.com/questions/45801457/node-js-python-not-found-exception-due-to-node-sass-and-node-gyp

Later versions of adm-zip might need a shim. Early version of node/react include some shims for fs/path ; so just upgrading to latest version of node seemed to break it.

I expect dva can be removed/replaced.

angrave commented 1 year ago

Experiment 1. Replacing node-sass with sass (a pure js version) but this will likely require a newer version of react-scripts. Simply removing and adding sass loader and sass was insufficient. Footnote: Updating node requires a new version of node-sass... but that causes issues with sass-loader (I think) https://github.com/sass/node-sass/issues/3103

nvm use 16
yarn add node-sass@6

but this causes

./src/screens/MediaSettings/index.scss
Error: Node Sass version 6.0.1 is incompatible with ^4.0.0.

Conclusion: Return to sass after we can update node and maybe react.


Experiment 2. I tried starting with a package.json from a new react 18 project and modern version of node (16) then incrementally adding each missing dependency (yarn add XYZ, yarn build...). Lessons learned: I should have started with react 16 not react 18; there are many packages that depend on react 16. The adm-zip filesystem (node/electron) dependency on fs or path does not go away. I assume this error message is relevant -

Module not found: Error: Can't resolve 'path' in '/home/research/classtranscribe/FrontEnd/node_modules/adm-zip'
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: { "path": require.resolve("path-browserify") }'
    - install 'path-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
    resolve.fallback: { "path": false }

I don't knowhow to add the shims back in that apparently we had in the current production build; The new project did not have a webpack.config.js. Adding items to package.json or reducing the adm-zip to "0.4.7 did not help. Though using the earl version complained about missing "fs" instead of 'path'

This may be relevant https://blog.logrocket.com/versatile-webpack-configurations-react-application/

Conclusion: Let's put aside upgrading to react18 until later.


Experiment 3 - Remove yarn.lock (then yarn install & yarn build) causes

./node_modules/react-use-set/dist/index.mjs
Can't import the named export 'useCallback' from non EcmaScript module (only default export is available)

Conclusion: Looks like we should be more specific on the version of react-use-set in package.json, as a minor version bump currently breaks the build.[Update: Nope original version does not even use this dependency]


Experiment 4 - Update the react-scripts to 4.0.3 Some build issues with lint stuff (which I ignored for this tes by deleting .eslintrc and deleting the lint stuff in packages.json ) Builds! Conclusions: This worked. Next steps: Address the lint, then maybe revisit using sass?

    "eslint-config-airbnb": "^19.0.4",
    "eslint-plugin-import": "^2.25.3",
    "eslint-plugin-react": "^7.28.0",
    "eslint-plugin-react-hooks": "^4.3.0"
angrave commented 1 year ago
angrave commented 1 year ago

@HanBoyou Woohoo - finally something easy - I removed "npm" from dependencies! Cant see why Node Package Manager should be bundled into the app :-) See fd7eba5407243137da286040151bef4599dbcf5b

(This means I can also remove the suggested PR to bump up npm https://github.com/classtranscribe/FrontEnd/pull/575 ) Generated a PR - works locally - hope to test deploy this soon.

angrave commented 1 year ago

@HanBoyou Okay I merged the PR but it seems that staging builds are now broken (i.e. does not pass lint checks when built using github actions).

Please can you take take a fresh copy of staging branch from github and take a look at it? (i.e. reinstall all dependences ; I'd like a PR that passes lint checks and appears to work). You can run the lint check locally with "yarn lint -q" I believe.

In the worst case, we can roll-back your latest PR... but I'd rather not do that...

HanBoyou commented 1 year ago

Working on this

harsh183 commented 5 months ago

Closing this for now since we

React Update is likely possible and it's being tracked here: https://github.com/classtranscribe/FrontEnd/issues/818

Moving dvaJS will be slightly more complicated, and it's being tracked here: https://github.com/classtranscribe/FrontEnd/issues/768