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

build: fix path-to-regexp to older version due to node 14 requirement #475

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

alvarowolfx
Copy link
Contributor

Sinon depends on the nise package. That package just published v6.0.1 which updated path-to-regexp from v6.2.1 to v8.1.0 with sinonjs/nise#226. That should have been a major version bump, as this ended up introducing a breaking change that now it depends on Node >= 16, which is causing our CI to break.

This PR fixes path-to-regexp to ^6.2.1 like previously to make it work on Node >= 14

@alvarowolfx alvarowolfx requested review from a team as code owners September 10, 2024 18:20
@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 10, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 10, 2024
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. labels Sep 10, 2024
@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 10, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 10, 2024
@fatso83
Copy link

fatso83 commented Sep 10, 2024

That should have been a major version bump

Sorry it this broke your build, but we don't support Node versions that old, so according to our published compatibility spec we did not break any contract we adhere to, and as such it should not have been a major version bump, as it did not break any Node engine versions we support for the current version of Sinon. We do try to enforce bumping the major any time we drop support for a previous Node version, though (such as dropping Node 16 -> Sinon 17 bump).

This PR was referenced from our issue tracker, so that's why I noticed, and thought I would clarify our stance on what constitutes a major version for us.

@alvarowolfx
Copy link
Contributor Author

@fatso83 thanks for the insights on that. We are figuring out how to fix that on our end, but basically it seems like all of our public Node GCP SDK are going to be blocked by that. I already notified our broad JS team here about this, to avoid duplicate work around that.

@fatso83
Copy link

fatso83 commented Sep 10, 2024

Ah, that is unfortunate, indeed. I think the underlying dependency is not that invasive, and could probably be replaced by something else (like https://github.com/marvinhagemeister/fast-path-to-regexp ?), though I haven't tried at all. If you have any resources to look into sending a patch that would fix your issue while not re-introducing the CVE that was fixed, I am of course totally open to that.

EDIT: a different take we thought of many years ago we did ponder splitting up Sinon into smaller, focused builds, allowing to drop stuff you did not need, which would have allowed some kind of sinon-minimal without the fake timers and fake-servers, and then manually including those if required. No one ever put in the hours to do that, but it's still possible.

@alvarowolfx alvarowolfx merged commit e05f07c into googleapis:main Sep 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. size: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants