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

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

Closed
angrave opened this issue Jun 15, 2023 · 6 comments
Closed

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

angrave opened this issue Jun 15, 2023 · 6 comments
Assignees

Comments

@angrave
Copy link
Collaborator

angrave commented Jun 15, 2023

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
Copy link
Collaborator Author

angrave commented Jun 16, 2023

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) sass/node-sass#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"

@HanBoyou HanBoyou self-assigned this Jun 18, 2023
@angrave
Copy link
Collaborator Author

angrave commented Jun 21, 2023

  • Update react scripts 3.x
  • Update react to latest 16.x that works.
  • What do we need in order to move to later version of material?
  • Can we replace node-sass with sass?
  • What needs to happen to remove DVA?
  • What other minor packages can we update?

@angrave
Copy link
Collaborator Author

angrave commented Jun 22, 2023

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

(This means I can also remove the suggested PR to bump up npm
#575
)
Generated a PR - works locally - hope to test deploy this soon.

@angrave
Copy link
Collaborator Author

angrave commented Jul 18, 2023

@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
Copy link

Working on this

@harsh183
Copy link
Member

Closing this for now since we

  • Add warning ignore in webpack config (Fix: #734) #817 - fixed the adm-zip issue
  • now use normal sass
  • use node 18
  • we now use react 16 that's quite widely used and well supported
  • upgraded to react scripts v4, and then ejected out of react scripts
  • the ejection also allows us to access webpack config to do whatever we want now

React Update is likely possible and it's being tracked here: #818

Moving dvaJS will be slightly more complicated, and it's being tracked here: #768

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

No branches or pull requests

3 participants