-
Notifications
You must be signed in to change notification settings - Fork 1
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
Build with Node 18 #105
Build with Node 18 #105
Conversation
3218f9d
to
4ddc810
Compare
Bump the Dockerfile to Node 18. Only install production dependencies. Bump the package file to Node 18.
4ddc810
to
6956b58
Compare
@@ -1,4 +1,7 @@ | |||
FROM node:14-slim as builder | |||
FROM node:18-slim as builder |
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.
I think this can be changed to node:18-alpine
, to make the image smaller.
ARG NODE_ENV=production | ||
ENV NODE_ENV=$NODE_ENV |
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.
These should ensure that Webpack etc. isn't installed into the deployed image. It will install React and MobX State Tree, which aren't needed by the proxy server, so can probably be improved.
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.
PR Review
This PR updates the code to build with Node 18 (this mostly affects the Proxy Server), and requires Node 18 to run local development.
- Also,
node-fetch
is no longer required! I didn't realise that Node 18 added the Fetch API, but it's very welcome.
Tested by running/testing 1. the web app and 2. the proxy server (example URL).
LGTM 👍
node-fetch
.