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

Minor regression in types params names with 4.20240903.0 #2652

Open
Cherry opened this issue Sep 4, 2024 · 6 comments
Open

Minor regression in types params names with 4.20240903.0 #2652

Cherry opened this issue Sep 4, 2024 · 6 comments
Assignees
Labels
types Related to @cloudflare/workers-types

Comments

@Cherry
Copy link
Contributor

Cherry commented Sep 4, 2024

Diff: https://npmdiff.dev/@cloudflare%2fworkers-types/4.20240821.1/4.20240903.0/package/experimental/index.ts
Raw diff: Cloudflare-Mining/Cloudflare-Datamining@66e9584#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:

  • URL.canParse
  • URL.parse
  • IdentityTransformStreamQueuingStrategy
  • URLSearchParams
  • URLSearchParams.append
  • URLSearchParams.delete
  • URLSearchParams.get
  • URLSearchParams.getAll
  • URLSearchParams.has
  • URLSearchParams.set
@Cherry Cherry added the types Related to @cloudflare/workers-types label Sep 4, 2024
@anonrig
Copy link
Member

anonrig commented Sep 4, 2024

@jasnell Any idea what caused this regression?

@penalosa
Copy link
Collaborator

penalosa commented Sep 4, 2024

@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
Copy link
Member

anonrig commented Sep 4, 2024

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

@jasnell
Copy link
Member

jasnell commented Sep 4, 2024

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

@jasnell
Copy link
Member

jasnell commented Sep 4, 2024

@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
Copy link
Contributor Author

Cherry commented Sep 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Related to @cloudflare/workers-types
Projects
Status: Backlog
Development

No branches or pull requests

6 participants