search

googleapis / nodejs-bigquery-storage

BigQuery Storage Node.js client
Apache License 2.0
34 stars 17 forks source link

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

Closed alvarowolfx closed 2 months ago

alvarowolfx commented 2 months ago

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 https://github.com/sinonjs/nise/pull/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

fatso83 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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.