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

fix: c3, use latest types for @cloudflare/workers-types #6897

Closed
wants to merge 1 commit into from

Conversation

threepointone
Copy link
Contributor

We have some code that looks for the latest compat date generated in a workers-types dist, but that's been frozen for 202307-01 forever. We have upcoming work that'll replace this with generated types based on compat date/flags, but till then let's generate new projects with the default types.

What this PR solves / how to test

Fixes #000

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Changeset included
    • Changeset not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: no functional change

@threepointone threepointone added the e2e Run e2e tests on a PR label Oct 5, 2024
Copy link

changeset-bot bot commented Oct 5, 2024

🦋 Changeset detected

Latest commit: a017bbb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-cloudflare Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Oct 5, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-wrangler-6897

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6897/npm-package-wrangler-6897

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-wrangler-6897 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-create-cloudflare-6897 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-cloudflare-kv-asset-handler-6897
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-miniflare-6897
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-cloudflare-pages-shared-6897
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-cloudflare-vitest-pool-workers-6897
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-cloudflare-workers-editor-shared-6897
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11194410995/npm-package-cloudflare-workers-shared-6897

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.80.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240925.0
workerd 1.20240925.0 1.20240925.0
workerd --version 1.20240925.0 2024-09-25

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@threepointone threepointone marked this pull request as ready for review October 5, 2024 07:41
@threepointone threepointone requested review from a team as code owners October 5, 2024 07:41
Copy link
Contributor

@Cherry Cherry left a comment

Choose a reason for hiding this comment

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

Using @cloudflare/workers-types without a version isn't the latest - it's in fact the oldest, so will be missing lots of stuff like stream constructors, getSetCookie, FormData parsing for File, global navigator and more.

Perhaps it would make more sense to default to /experimental which is the latest, until said time the generated types are implemented?

@threepointone
Copy link
Contributor Author

good catch, yes

We have some code that looks for the latest compat date generated in a workers-types dist, but that's been frozen for 202307-01 forever. We have upcoming work that'll replace this with generated types based on compat date/flags, but till then let's generate new projects with the default types.
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

oof.. yes, this is quite out of date, but I'm not sure if using the /experimental entry point is the right intermediate step as that could result in unexpected breaking changes that can be quite tricky for people to debug and understand their source. This approach seems very fragile to me.

it might be better to instead introduce one more versioned/dated entry point into workers-types and update to that as an intermediate step towards dynamically generated types.

@andyjessop, @penalosa, and @petebacondarwin will likely have opinions/preferences on the next steps too.

@DaniFoldi
Copy link
Contributor

Hi @IgorMinar,

I don't really see a problem with the current setup in main - it looks up the date correctly, and finds 2023-07-01, which, despite being over a year ago, is the current newest types entrypoint. A new entrypoint in workers-types is only needed if there is a compat flag with an enable date that influences types.
I am fairly confident that the last time I checked (early September) this was still the same as 2023-07-01, this is something I check regularly - we use experimental in our projects for a number of reasons but hopefully that can be replaced by the proper type generation (especially if it includes types for nodejs_compat).

Is there something I'm missing here, or perhaps this would break the upcoming work mentioned?

@petebacondarwin
Copy link
Contributor

@DaniFoldi - this was my take on this too. I think the problem was that the current "latest" compat date seemed very old which made it look like it was wrong.

That being said we actually did land a new compat date in 2024-09-23, which causes nodejs_compat to infer nodejs_compat_v2. I am not sure if this should have a new workers-types directory, in which case there is a bug in our types generation, or not, since this compat date change is a bit unusual and new.

I would actually go further and say that changing the entry-point to something "generic" like @cloudflare/workers-types/latest could cause future problems since the user might then update @cloudflare/workers-types and pull in new types without updating their compat date.

The most effective solution I believe is to move away from @cloudflare/workers-types altogether and use the new automated type generation added in #6295. This makes the compat date in wrangler.toml the source of truth and avoids unexpected skew.

@DaniFoldi
Copy link
Contributor

Hi @petebacondarwin,

Thanks for the Sunday response! I've looked at the flags again, and maaaybe

  • 2023-12-01 should have been one, although it looks like the generated types don't change between experimental and 2023-07-01 in that area (RSA publicExponent).
  • vectorize should have gotten a new entrypoint for the metadata change, although I seem to recall discussing that with the team and they argued that it was in beta at the time, so no flag needed.

We've actually discussed this with @mrbbot in quite a lot of detail back when this system was introduced, and we both agreed that it's suboptimal due to the separate "source of truth" for date, and no way to configure the exact flags used.

This was before nodejs_compat was introduced, and the differences were minor, so most people, just using a date were fine. After the original nodejs_compat was introduced, I was hopeful that there would be a +nodejs_compat variant of the entrypoints, especially when the date system was introduced with v4. Unfortunately, that day never came and @types/node is still needed for nodejs_compat-enabled workers to typecheck successfully, even though that introduces more types than the workerd runtime supports, providing a false sense of "so it will work on Workers" when using node APIs.

This is why 2024-09-26 does not require a new entrypoint, because other than the nodejs stuff, native runtime types are identical.

My original idea for solving the entrypoint problem (to a degree) was to generate an entrypoint for every combination that was different. Then, exports in package.json could be used to route to the correct one, like @cloudflare/workers-types/2024-10-01+nodejs_compat for a recent worker. This could then be used with an export pattern like "2024-10-*+nodejs_compat": "maybe-a-hash-or-the-earliest-date.d.ts". With patterns, I believe the number of files wouldn't grow too quickly, as in quite a few months, nothing changes and only one entry would be needed, with no added file.


Now, with the type generation system that can maybe replace workers-types and improve this experience, perhaps it would be useful to actually support nodejs_compat and nodejs_compat_v2 (my idea there is to mark shims as deprecated so developers know they won't actually do useful work at runtime) so @types/node isn't needed either.

@petebacondarwin
Copy link
Contributor

Now, with the type generation system that can maybe replace workers-types and improve this experience, perhaps it would be useful to actually support nodejs_compat and nodejs_compat_v2 (my idea there is to mark shims as deprecated so developers know they won't actually do useful work at runtime) so @types/node isn't needed either.

We are actually close to solving that problem with generated types to!
See cloudflare/workerd#2708 and #6723

@penalosa
Copy link
Contributor

Closing in favour of #6883

@penalosa penalosa closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants