Update Node and dependencies to work on Apple M1 #39
Conversation
|
Also worth noting here: I had to update |
| @@ -43,8 +45,6 @@ psql -h localhost -p 5432 | |||
|
|
|||
| Create .env file in server workspace based on the .env.example. See Deployment section for more information on the .env file. | |||
|
|
|||
| Also create .env file in client workspace based on the .env.example. | |||
There was a problem hiding this comment.
Actually this client .env file does exist and is required for the app to run! The example file is named confusingly (https://github.com/usdigitalresponse/usdr-gost/blob/main/packages/client/.env.development), I'll adjust that in a follow-on PR or feel free to rename it to env.example as part of this changeset.
There was a problem hiding this comment.
So FWIW I don't think the naming was confusing at all, but may require more familiarity with dotenv than some folks have (?). Dotenv will pick up any .env or .env.$NODE_ENV file, so it will pick up the existing .env.development file just fine without you needing to create a new .env (at least it did so quite reliably for me, development is the default value if you don’t have NODE_ENV set). i.e. .env.development is not an example, or at least shouldn’t be treated like one since dotenv will actually load it by default in your development environment.
Since you don't actually need to do anything here, I thought removing the instruction (which inaccurately references .env.example to boot) made the most sense.
There was a problem hiding this comment.
(If curious, this is a design choice inherited from the original Ruby dotenv gem, described here: https://github.com/bkeepers/dotenv#can-i-use-dotenv-in-production and you can see how NPM dotenv’s docs, which are newer, discourage this usage, described here: https://github.com/motdotla/dotenv#should-i-commit-my-env-file)
There was a problem hiding this comment.
That makes sense! Does it actually work for you locally? When I first set up this project I had to create a .env file in the client package in order for the web app to load, with an explicit NODE_ENV set to development. I'll try remove it as it sounds like it shouldn't be necessary.
There was a problem hiding this comment.
@Mr0grog just tested building out the client without a .env file in my local build and the app doesn't load even when in development mode. It works when a .env file is present.
I'm obviously not deeply familiar with dotenv and the mechanics here but wanted to flag that since I needed to add this file manually when doing my initial set up; perhaps there's a way to make this seamless so the next person to walk through this README can build successfully
There was a problem hiding this comment.
Does it actually work for you locally? …just tested building out the client without a .env file in my local build and the app doesn't load even when in development mode.
Weird! It worked great for me. I never created a .env file.
There was a problem hiding this comment.
Maybe the best solution here is to rename the file to .env.example and retain the instruction. (As a side note, I generally don’t think it’s good to commit .env files with values that are intended to be used, so I think this would be better overall anyway!)
There was a problem hiding this comment.
🎉 That seems great. I wonder what is happening here for me locally 😬
|
You are a hero for going through and updating a few of these archaic packages. 🙇♀️ Thank you for inching us towards working with 2022 compatible artifacts. I think the next meaningful step along these lines will be tackling #45 so we can upgrade Vue + Node and then everything else. I have been running this app on an M1 for the past month or so using node 15 but the chromedriver workarounds were painful and I'm not going to bother walking you through them as they are unnecessary after #44 is merged. I'm going to go ahead and merge the chromedriver removal now. If you don't mind doing three small changes I think this should be ready to merge:
|
|
Re: Node.js versions, there’s probably no reason not to upgrade to 16.x if 14.x is problematically old! I was going for minimum update in this PR, which is the only reason I chose 14 instead of 16 here. 16 is probably better over the long run anyway since it is the current LTS release. |
|
@Mr0grog I recently tried to upgrade the project to node 16 but found a dependency related to Vue.js version 2 that was gated on node 15 or lower. Feel free to give it a try, but I know you've sunk a lot of time into this already, so I think it's fine if that waits for #45. There is SO MUCH to upgrade in this repo and getting on a more modern version of Vue should unblock all of it once the app has been updated to be compatible with the new version. |
Ahhhh, I think you mentioned this before and I forgot. So it seems like we should just leave this part of the PR as-is. 👍 |
|
Yeah I think it's ready to go 👍 |
|
OK, so what’s left here is:
I won’t get back to this until next week, so feel free to finish these out yourself if you’d like to get it done sooner! |
The current dependencies and version of Node.js do not support Apple M1 processors. These are the minimum updates needed to work. (But not the maximum updates! For example, Node.js 14 is past its end-of-life, and Chromdriver is many major versions out of date.)
cmroberts
force-pushed
the
i-tried-to-set-up-this-project-and-i-had-some-problems
branch
from
291fa0a
to
b1a6a0a
Compare
cmroberts
deleted the
i-tried-to-set-up-this-project-and-i-had-some-problems
branch
I tried to set up this project (in order to add a simple CI workflow) and I couldn’t since the version of Node and some dependencies wasn’t compatible with my machine. These are the minimum updates to fix the issue (they’re all at least a major version behind what’s current).
I also cleaned up a few things related to basic setup:
package-lock.jsonfiles since this project uses Yarn..node-versionfile for nodenv users, which is just a symlink to the.nvmrcfile. There’s a risk this could cause issues on Render, or not work correctly for Windows (but WSL should be fine) users with nodenv..env.examplefile in theclientpackage, which does not actually exist.I did not actually wind up setting up a CI workflow here, since I still can’t get any of the tests to work. That doesn’t appear to be a problem with dependencies or Node, though — just broken tests or config.