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

Fix issues creating prototypes when using npm v10.4.0 or newer by removing dependency on zlib #2404

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

joelanman
Copy link
Contributor

@joelanman joelanman commented Mar 20, 2024

the kit had a dependency of zlib 1.0.5 - this is 13 years old, and is replaced by the built in zlib in node itself

this should fix this issue that users have reported:

kkaefer/DEPRECATED-node-zlib#14

fixes #2405

@oscarduignan
Copy link

oscarduignan commented Mar 28, 2024

just adding a comment here for people if they need a work around, assuming you're using a version of npm (and that's npm, not, nodejs - the versioning is different) greater than 8.3.0 (which introduced overrides) then you can exclude zlib by adding this into your package.json and trying running npm install again:

"overrides": {
  "zlib": "./_EXCLUDED_"
}

@joelanman
Copy link
Contributor Author

@oscarduignan thanks! I've updated my template repo, as the npx command will fail:

https://github.com/joelanman/govuk-prototype-kit-prototype

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look fine but I think that it'd be useful to:

  • capture why we're making the change in the commit message
  • explain the impact of the change in the changelog entry

Would you be happy to pick that up? If you prefer I can update the commit, PR title and changelog entry as long as you're happy for me to edit your wording!

@joelanman
Copy link
Contributor Author

@36degrees cheers, happy for you to edit any of it!

@36degrees 36degrees changed the title Remove old zlib in favour of built in Fix issues creating prototypes when using npm v10.4.0 or newer by removing dependency on zlib Apr 3, 2024
joelanman and others added 2 commits April 3, 2024 15:43
Creating new prototypes is (silently) failing when using npm v10.4.0 or newer because of an error that occurs when installing the `zlib` package:

```
$ npm install zlib
npm ERR! code 127
npm ERR! path /Users/oliver.byford/Code/zlib-test/node_modules/zlib
npm ERR! command failed
npm ERR! command sh -c node-waf clean || true; node-waf configure build
npm ERR! sh: node-waf: command not found
npm ERR! sh: node-waf: command not found
```

The `zlib` package is 13 years old, and has been replaced by a zlib ‘core module’ built into node itself since Node v0.6.

`require(‘zlib’)` will always return the core module [1] over a dependency from `node_modules`, and so despite being listed as a dependency the `zlib` package will never be used [2] and can safely be removed.

[1]: https://nodejs.org/api/modules.html#modules_all_together
[2]: kkaefer/DEPRECATED-node-zlib#7

Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR title and changelog entry and added some more context to the commit message.

Needs a second review before we can get this merged.

Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@36degrees 36degrees merged commit 3d8e7fc into alphagov:main Apr 3, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kit can fail to run, with zlib errors
4 participants