cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.32k stars 311 forks source link

Minor regression in types params names with `4.20240903.0` #2652

Closed Cherry closed 2 months ago

Cherry commented 3 months ago

Diff: https://npmdiff.dev/@cloudflare%2fworkers-types/4.20240821.1/4.20240903.0/package/experimental/index.ts Raw diff: https://github.com/Cloudflare-Mining/Cloudflare-Datamining/commit/66e958488df276e1d052cdb93e13b365e20fbd22#diff-498f7d22754cd92e5654b3be0adcae3df0c297abe440099d42dbc8cf43af66fd

FixedLengthStream previously had named paramaters:

declare class FixedLengthStream extends IdentityTransformStream {
  constructor(
    expectedLength: number | bigint,
    queuingStrategy?: IdentityTransformStreamQueuingStrategy,
  );
}

Now these are more generic with just param1 and param2:

declare class FixedLengthStream extends IdentityTransformStream {
  constructor(
    param1: number | bigint,
    param2?: IdentityTransformStreamQueuingStrategy,
  );
}

A few more methods/classes had the same thing occur including:

anonrig commented 3 months ago

@jasnell Any idea what caused this regression?

penalosa commented 3 months ago

@jasnell @anonrig have the implementations for streams or URLs moved? We generate parameter names by parsing the c++ AST so it's relatively sensitive to refactoring

anonrig commented 3 months ago

The last change seems to be clang-format https://github.com/cloudflare/workerd/commits/main/src/workerd/jsg/url.c%2B%2B

jasnell commented 3 months ago

No changes that I'm aware of that would cause this. Really odd. Could the formatting changes have broken the ast transform?

jasnell commented 3 months ago

@penalosa ... I think we should first rule out a regression in the type generation logic. If that points to a change on the runtime side, then we'll investigate further but there really hasn't been much here that could have changed as far as I know.

Cherry commented 2 months ago

It looks like this was partially resolved in the latest update:

https://npmdiff.dev/@cloudflare%2fworkers-types/4.20240909.0/4.20240919.0/package/experimental/index.ts

FixedLengthStream has the correct param names again, as well as IdentityTransformStream, but not any of the others yet.