alphagov / govuk-prototype-kit

Rapidly create HTML prototypes of GOV.UK services
https://prototype-kit.service.gov.uk
MIT License
307 stars 237 forks source link

npm install on node 14 includes optional dependencies #1354

Closed joelanman closed 2 years ago

joelanman commented 2 years ago

Description of the issue

Running npm install on node 14 on a mac installs optional dependencies

Steps to reproduce the issue

Run npm install on node 14 on a mac

Actual vs expected behaviour

Install optional dependencies including Cypress, 1137 packages in total Expected only core dependencies

Environment (where applicable)

lfdebrux commented 2 years ago

What version of npm was this using?

joelanman commented 2 years ago

Might take a while to boot up the old macbook, probably worth trying with 6, 6.14 or later, according to https://nodejs.org/en/download/releases/. Might be worth covering the npm for node 12 too

lfdebrux commented 2 years ago

Hmm, I just found that for npm@8.7.0 using deprecated config variables will emit some very annoying warnings [1]. However, we use deprecated config variables to make sure users with older node installations don't install the dev dependencies by default.

So with the changes in #1373 for anyone who has installed the latest version of Node 16, or upgraded npm, they will see

npm WARN config optional Use `--omit=optional` to exclude optional dependencies, or
npm WARN config `--include=optional` to include them.
npm WARN config
npm WARN config     Default value does install optional deps unless otherwise omitted.

> govuk-prototype-kit@12.1.0 start
> node start

compiling CSS...⠂⠂⠂) ⠦ : timing config:load:flatten Completed in 3ms
compiling CSS...⠂⠂⠂) ⠦ : timing config:load:flatten Completed in 3ms
copying assets...⠂⠂) ⠦ : timing config:load:flatten Completed in 3ms
copying assets...
(⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂) ⠦ : timing config:load:flatten Completed in 3ms
GOV.UK Prototype Kit v12.1.0

NOTICE: the kit is for building prototypes, do not use it for production services.

Listening on port 3000   url: http://localhost:3000
(⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂) ⠦ : timing config:load:flatten Completed in 3ms

when they run the kit with npm run start, which is very confusing. This can also make the kit unusable, because the output timing config:load:flatten can interfere with the prompt asking whether you want to allow analytics.

We could try and hide the messages by stopping npm from logging warnings to the console, but this would have the unfortunate side-effect of hiding any legitimate warnings. It also might not work forever, as it appears that the plan is that in npm version 9 using deprecated config variables will be an error [2]. I couldn't find any information on when development on version 9 will start though, so it might not be something to worry about any time soon.

So I think that we can hide the warnings for now, but we should think about alternative strategies as well.

lfdebrux commented 2 years ago

Just by coincidence, had a support issue today where someone missed the warning from npm install and hadn't installed a peer dependency; this is one of the cases where hiding warnings from npm would have a negative outcome, because npm install only warns about peer dependencies.

The library that needed the peer dependency was @ministryofjustice/frontend.

lfdebrux commented 2 years ago

We've discussed, and we think the best thing is to reject the change in #1384 and revert the fix in #1373. This will mean no warnings for users of npm 8, but a less good experience for users of npm 6.

We could also add something to the release notes for the next release to suggest users update npm, so they get a better experience.

lfdebrux commented 2 years ago

This was properly fixed with our new release process in #1399.