-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bump Webpack to v5 and node to 18 #343
Conversation
6acee96
to
e2f8033
Compare
1b4812f
to
ee13f21
Compare
cf3cb8b
to
5cff68c
Compare
18.18.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, why upgrade to Node 18 over 20? Was this because of issues with whatwg-fetch
and url.parse()
? In our case would this throw errors rather than just warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, have bumped up to 20 now! 18 was the first LTS version I started having problems with so I was focused on getting that running, it seems that 20 is working now too.
Pushed to the wrong branch (the follow-up: https://github.com/guardian/atom-workshop/pull/344/files)
"@babel/preset-react", | ||
[ | ||
"@babel/preset-env", | ||
{ | ||
"useBuiltIns": "entry", | ||
"corejs": "3.19", | ||
"targets": { | ||
"chrome": 75, | ||
"ios": 12 | ||
} | ||
} | ||
] | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this config change needed due to upgrading webpack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the existing babel dependencies were incompatible - since I was moving from babel-core
et al to @babel/core
et all I needed to overhaul this file - mostly this is borrowed from Composer's .babelrc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and it builds and runs as expected. I tried creating an atom and publishing it, which works fine. But it doesn't appear when I search for it from the dashboard. Nor does it allow me to embed it in composer (I get the error:
This URL cannot be embedded.
If you have custom embed code, please paste it in and embed it instead. If this is a content atom, make sure that it has been published.
I imagine this might be something I'm missing with how to use Atom Workshop, rather than an issue with this PR, but maybe worth having a quick look together to be sure?
Very thorough! I reckon we should deploy to CODE and see if it works as expected there.
Thanks very much - I reckon we should deploy to CODE and see if it works as expected there. I wouldn't be surprised if the local development story is a bit messy. I couldn't get it to run before I made my changes. |
Yes I realised I could see atoms created on CODE when running locally so wondered how creating an atom locally might be integrating with these. Deploying to CODE sounds like a good plan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, works on CODE as expected!
What does this change?
This PR bumps Webpack to v5. Quite a few related packages also needed to be bumped (e.g. Babel, various loaders). Node is also bumped to
18
.This work started because
atom-workshop
was using node 12 - long out of LTS. Dependencies that required Webpack 2 were incompatible with higher versions of node - so in order to bump Node I had to also bump Webpack (deciding to get it up-to-date rather than leaving at 3 or 4).The Webpack API for declaring loaders has changed from v3 to v5 so I've made some changes to the Webpack
conf
file for that reason. I've removedfile-loader
andurl-loader
instances in favour of Webpack 5's "asset/resource" test. I've also removedimport-loader
andexport-loader
, which should sort a high priority vulnerability.ExtractTextPlugin
is now deprecated, so is replaced here withMiniCssExtractPlugin
.Jest needed to be bumped due to a package incompatibility - consequently the snapshots have needed to be updated, and a test needed to be updated as it was behaving differently after the package bump (it wasn't picking up on an error thrown asyncrously). I've also changed the arguments to our
lint
andtest
commands to apply to a more specific range of files - both were picking up files built in thetarget
directory.This PR also removes
webpack-webserver.conf.js
as it's been unused since #259I've added an
if
condition around the call tothis.props.updateFormErrors
inManagedEditorField
because a new React error log was happening when the function was called when it hadn't been defined - it was already not being passed as a prop in many places so I'm assuming this is to do with changes related to logging via either Babel or React version changes as the app is still functioning as before.Along with the previous PRs, this removes all but one of the high priority client-side vulnerabilities in atom-workshop (the remaining one in is Scribe, hopefully to be replaced with
prosemirror-editor
).How to test
Run the project locally according to the instructions in the readme. Does it build and run as expected? Do on-page assets seem to be getting bundled and loaded correctly?