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

Upgrade dependencies #440

Merged
merged 12 commits into from Nov 22, 2022
Merged

Upgrade dependencies #440

merged 12 commits into from Nov 22, 2022

Conversation

mroderick
Copy link
Member

Purpose (TL;DR) - mandatory

@fatso83
Copy link
Contributor

fatso83 commented Nov 21, 2022

Since rebasing and doing all the upgrades, Node 14 complains:

  npm ci
npm ERR! fsevents not accessible from chokidar

@mroderick
Copy link
Member Author

Since rebasing and doing all the upgrades, Node 14 complains

There's always one squeaky wheel, isn't there? :P

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 95.52% // Head: 95.52% // No change to project coverage 👍

Coverage data is based on head (288fb86) compared to base (51f0583).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files           2        2           
  Lines         648      648           
=======================================
  Hits          619      619           
  Misses         29       29           
Flag Coverage Δ
unit 95.52% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/fake-timers-src.js 95.51% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fatso83
Copy link
Contributor

fatso83 commented Nov 22, 2022

Fixed the Node 14 issue by adding fsevents to optional deps and recreating the lock file (using NPM 8). Got some hints at bahmutov/npm-install#103 (comment)

@fatso83 fatso83 merged commit 2f07944 into main Nov 22, 2022
@fatso83 fatso83 deleted the upgrade-dependencies branch November 22, 2022 16:00
@mroderick
Copy link
Member Author

Thank you ❤️

@@ -50,6 +50,9 @@
"dependencies": {
"@sinonjs/commons": "^2.0.0"
},
"optionalDependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

This means every (mac) consumer of this module will install this. Would be nice to figure out which dependency has messed up their dependency tree and fix that instead of hacking around it here

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimenB that's a fair point. Would you mind making a ticket for it, so we don't forget?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK when looking at this, it was chokidar. And this was only an issue with Node 14/NPM 6, not with any later version, so I was thinking it was more of an issue of it only being able to handle the v1 of the package-lock format, whereas the package-lock file has v2.

And I think we want all consumers of mac to install this? This package ensures it uses file system events instead of polling.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK when looking at this, it was chokidar. And this was only an issue with Node 14/NPM 6, not with any later version, so I was thinking it was more of an issue of it only being able to handle the v1 of the package-lock format, whereas the package-lock file has v2.

Could we upgrade the version of npm used on CI so the lockfile is supported? Or stay on v1 I guess (unless newer npm just updates the format automatically)

And I think we want all consumers of mac to install this? This package ensures it uses file system events instead of polling.

@sinonjs/fake-timers doesn't do any FS at all, tho, so it shouldn't come with fsevents (or chokidar).

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,5 @@
#!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"
Copy link
Member

Choose a reason for hiding this comment

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

does this work for you? I'm getting

image

when I try to commit

Copy link
Member

Choose a reason for hiding this comment

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

npm bin fails for me

image

using npx works - I can send a PR

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

whaaat. What kind of NPM are you running? npm bin works even if alias npm=pnpm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I need to test this locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Works fine? Is not npx potentially just a lot slower?

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.

None yet

3 participants