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

Allow Node 15 to be used #995

Merged
merged 4 commits into from
Mar 19, 2021
Merged

Allow Node 15 to be used #995

merged 4 commits into from
Mar 19, 2021

Conversation

frankieroberto
Copy link
Contributor

This upgrades the list of supported Node versions to include v15. This is useful as currently only version 15 is supported by Macs using the new M1 ARM chips (see nodejs/node#36161).

I've been running it locally using version 15 fine, and have upgraded our prototype to node 15 running on Heroku.

This upgrades the list of supported Node versions to include v15. This is useful as currently only version 15 is supported by Macs using the new M1 ARM chips (see nodejs/node#36161).

I've been running it locally using version 15 fine, and have [upgraded our prototype](DFE-Digital/apply-for-teacher-training-prototype#454) to node 15 running on Heroku.
@joelanman
Copy link
Contributor

Thanks @frankieroberto !

This might also need updates to the tests:

https://github.com/alphagov/govuk-prototype-kit/blob/master/.github/workflows/run-tests.yaml

I wonder if the test platform supports M1

@frankieroberto
Copy link
Contributor Author

May also be worth mentioning node 15 for Mac users here: https://govuk-prototype-kit.herokuapp.com/docs/install/requirements.md

Alternatively, recommend node 15 for all users (I think it's probably stable enough for prototyping use, and saves users having to know what type of MacBook they have)?

@36degrees
Copy link
Contributor

I wonder if the test platform supports M1

Unfortunately not, but as you say we can at least test that the kit runs on Node 15.

Alternatively, recommend node 15 for all users (I think it's probably stable enough for prototyping use, and saves users having to know what type of MacBook they have)?

I think we generally recommend the LTS releases as they are supported for a lot longer, and the Node doc on releases states that 'production applications should only use Active LTS or Maintenance LTS releases'. Whilst prototypes aren't production applications, I think the intention is that the odd-numbered releases are really only used by folks who want to live on the 'cutting edge'.

My preference would be to add a note to the install docs for M1 users. We could look at simplifying it by recommending Node 16 a little earlier, rather than waiting until Active LTS support starts in October?

@frankieroberto
Copy link
Contributor Author

@36degrees:

My preference would be to add a note to the install docs for M1 users. We could look at simplifying it by recommending Node 16 a little earlier, rather than waiting until Active LTS support starts in October?

Sounds good. I'm guessing it'll be a little while before there are lots of work M1 MacBooks being issued, and hopefully by then node 16 will be out. In the meantime, recommending node 14 but allowing 15 to be used would be ok.

@joelanman
Copy link
Contributor

does the engines change actually affect end users? My vague recollection is that it doesnt, and is more for test suites

I think we might need to target end users via the install docs?

Is there a specific error that happens that we could mention?

@frankieroberto
Copy link
Contributor Author

@joelanman yeah, npm install will halt if the current version of node isn't compatible with the versions listed in engines with the error "Unsupported engine". I can't remember which version of npm added this check, but I think it's been around for a little while. You can also list which versions of npm itself are supported within engines.

@frankieroberto
Copy link
Contributor Author

The error you get when trying to install node < 15 on an M1 mac is very cryptic, referring to compile errors and so on.

@36degrees 36degrees added this to the v9.13.0 milestone Mar 10, 2021
@joelanman
Copy link
Contributor

there is more we can do to support users, but I think we should probably just merge this to stop blocking 15. @frankieroberto do you mind adding a line to the Changelog? I can help if you'd like

@frankieroberto
Copy link
Contributor Author

@joelanman I've added a note to the changelog and to the installation documentation. Let me know if there’s anything else needed for a merge. 👍

@joelanman
Copy link
Contributor

thanks @frankieroberto ! Really sorry but do you mind reverting the guidance change? For now I think we'll just allow it via package.json, we need to think a bit more about the guidance on the team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants