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

feat: add runtime type generation to wrangler types command #6295

Merged
merged 12 commits into from
Jul 23, 2024

Conversation

andyjessop
Copy link
Contributor

@andyjessop andyjessop commented Jul 19, 2024

This PR introduces dynamic runtime type generation, according to a user's project configuration. The feature is hidden behind an experimental flag (--x-with-runtime), so is added here as a minor version change.

Why is this required?

User's can specify compatibility_flags for their project, but our @cloudflare/workers-types package does not take these into account. It only accounts for the compatibility_date, and users specify the entry point in their tsconfig.json's compilerOptions.types array, e.g. `"types": ["@cloudflare/workers-types/2024-01-01"]. Clearly we can't produce entry points for every possible combination of date and flags.

What is the solution?

The workerd npm package now includes a worker (workerd/worker.mjs) that accepts compatibility date and flags in the URL path, e.g.

http://my-worker.com/2022-01-01+flag_one+flag_two

The response is a string of TS types that can be written to the filesystem for use in TS projects.

This PR hooks up wrangler types --x-with-runtime to this type-generating worker, providing it with the config from the user's project, and saving either to a default location:

# Generates types and saves to .wrangler/types/runtime.d.ts
npx wrangler types --x-with-runtime

Or to a specified location:

# Generates types and saves to ./types.d.ts
npx wrangler types --x-with-runtime="./types.d.ts"

Note that the default location is inside the .wrangler hidden folder, so this file will not be committed by default. In order to commit this file, the user can specify a location that is not ignored by git.

Advice is given in the console to:

  • uninstall @cloudflare/workers-types as it's no longer necessary
  • to get node types, as they're not included here yet

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required / Maybe required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Not necessary because: This is an experimental feature, so will be quietly released through community channels.

Copy link

changeset-bot bot commented Jul 19, 2024

🦋 Changeset detected

Latest commit: a2eed89

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

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers 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

@workers-devprod workers-devprod added the e2e Run e2e tests on a PR label Jul 19, 2024
Copy link
Contributor

github-actions bot commented Jul 19, 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/10055066450/npm-package-wrangler-6295

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

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

Or you can use npx with this latest build directly:

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

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


wrangler@3.65.1 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240718.0
workerd 1.20240718.0 1.20240718.0
workerd --version 1.20240718.0 2024-07-18

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

const updatedTypesString = buildUpdatedTypesString(existingTypes, outFile);

const message = isWorkersTypesInstalled
? `\n📣 Replace the existing "@cloudflare/workers-types" entry with the generated types path:`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is the convention for using emojis here. Without them it's a little dull and harder to read, but maybe we have another way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Emojis are fun, so this seems fine to me? 🤷

@@ -468,6 +487,7 @@ function writeDTSFile({
combinedTypeStrings,
].join("\n")
);
logger.log(`Generating project types...\n`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I referred to this as "project types", but open to suggestions of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it match the --x-with-runtime arg and say "Generating runtime types..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not for the runtime types, but for the Env types, so the wording is for distinguishing between the two.

@andyjessop andyjessop marked this pull request as ready for review July 19, 2024 12:02
@andyjessop andyjessop requested a review from a team as a code owner July 19, 2024 12:02
fixtures/runtime-types/.editorconfig Outdated Show resolved Hide resolved
fixtures/runtime-types/.gitignore Outdated Show resolved Hide resolved
fixtures/runtime-types/.prettierrc Outdated Show resolved Hide resolved
fixtures/runtime-types/package.json Outdated Show resolved Hide resolved
packages/miniflare/package.json Outdated Show resolved Hide resolved
packages/wrangler/e2e/types.test.ts Outdated Show resolved Hide resolved
packages/wrangler/e2e/types.test.ts Outdated Show resolved Hide resolved
packages/wrangler/e2e/types.test.ts Outdated Show resolved Hide resolved
packages/wrangler/scripts/deps.ts Show resolved Hide resolved
chore: remove unused file

chore: add changeset

chore: remove unnecessary fixture

chore: add try/catch to JSON.parse

chore: update changeset

chore: update changeset and isolate tests

chore: revert pnpm lock changes
packages/wrangler/src/type-generation/runtime/index.ts Outdated Show resolved Hide resolved
packages/wrangler/src/type-generation/runtime/index.ts Outdated Show resolved Hide resolved
packages/wrangler/src/type-generation/runtime/index.ts Outdated Show resolved Hide resolved
packages/wrangler/src/type-generation/runtime/index.ts Outdated Show resolved Hide resolved
const updatedTypesString = buildUpdatedTypesString(existingTypes, outFile);

const message = isWorkersTypesInstalled
? `\n📣 Replace the existing "@cloudflare/workers-types" entry with the generated types path:`
Copy link
Contributor

Choose a reason for hiding this comment

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

Emojis are fun, so this seems fine to me? 🤷

Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

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

Looks good to me! One additional thing that would be good to see is some sort of comment header at the top of the generated types file that explains what compatibility data was used in its generation, for future optimisations. Something like // Auto generated runtime types: workerd@1.20240101.0 /20230102+global_navigator

Comment on lines 1 to 2
// Generated by Wrangler on Thu Mar 28 2024 11:27:36 GMT+0000 (Greenwich Mean Time)
// by running `wrangler types`
// Generated by Wrangler on Mon Jul 22 2024 10:41:44 GMT+0200 (Central European Summer Time)
// by running `wrangler types --x-with-runtime`
Copy link
Contributor

@RamIdeas RamIdeas Jul 22, 2024

Choose a reason for hiding this comment

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

Would be great if this file was reproducible without a diff each time wrangler types was run.

Is the timestamp necessary? I think @penalosa's idea of using the workerd version and compat date+flags would work better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I like that better. Not sure why we have the timestamp.

Unless it's specifically to create a diff? Maybe we're undoing a specific decision by removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking back through the history, and the original PR that introduced this, there doesn't seem to be a specific reason to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RamIdeas @penalosa can I suggest doing this in a follow-up PR? It requires some refactoring so might be better of as an atomic set of changes.

throw new Error(text);
}

await mf.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call this in a try..finally so that the workerd instance is guaranteed to be killed if any unexpected errors occur

Copy link
Contributor

@RamIdeas RamIdeas left a comment

Choose a reason for hiding this comment

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

LGTM! One note about the flag name: –x-with-runtime. Rather than "with" can we go with "include": --x-include-runtime?

@andyjessop
Copy link
Contributor Author

andyjessop commented Jul 22, 2024

LGTM! One note about the flag name: –x-with-runtime. Rather than "with" can we go with "include": --x-include-runtime?

Can do! Is there any precedent for either option, or are we just choosing something that makes sense here?

@RamIdeas
Copy link
Contributor

LGTM! One note about the flag name: –x-with-runtime. Rather than "with" can we go with "include": --x-include-runtime?

Can do! Is there any precedent for either option, or are we just choosing something that makes sense here?

Just thinking about this becoming non-experimental and then on-by-default and then using this flag to disable the runtime types --no-with-runtime vs --no-include-runtime

@petebacondarwin
Copy link
Contributor

LGTM! One note about the flag name: –x-with-runtime. Rather than "with" can we go with "include": --x-include-runtime?

Can do! Is there any precedent for either option, or are we just choosing something that makes sense here?

Just thinking about this becoming non-experimental and then on-by-default and then using this flag to disable the runtime types --no-with-runtime vs --no-include-runtime

Just one thing to consider. I would naturally avoid encouraging people to use the --no- prefix option for turning off most flags. Instead I would suggest --include-runtime=false/--with-runtime=false, which works for either wording.

@andyjessop andyjessop merged commit ebc85c3 into main Jul 23, 2024
20 checks passed
@andyjessop andyjessop deleted the aj/runtime-types-mf branch July 23, 2024 08:44
dario-piotrowicz pushed a commit that referenced this pull request Jul 28, 2024
* feat: add runtime type generation to wrangler types command

chore: remove unused file

chore: add changeset

chore: remove unnecessary fixture

chore: add try/catch to JSON.parse

chore: update changeset

chore: update changeset and isolate tests

chore: revert pnpm lock changes

* chore: update lock file

* chore: re-add vitest config

* chore: check for node compat, extract util file

* chore: update node_compat logic and wording

* chore: reuse node_compat logic

* chore: revert order

* chore: fix mode

* chore: update e2e test

* chore: dispose of Miniflare inside 'finally' block

* chore: add more tests

* chore: use --x-include-runtime instead of --x-with-runtime

---------

Co-authored-by: Andy Jessop <ajessop@cloudflare.com>
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
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants