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: graduate --x-include-runtime #8166

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

emily-shen
Copy link
Contributor

@emily-shen emily-shen commented Feb 17, 2025

TODO: rebase on /vnext for inclusion in wrangler v4

Might be best to review commit by commit

  • DEVX-1615 improve logging output: d33b18a, 1b0d071
  • DEVX-1611 remove --x-include-runtime and consolidate type output into one file: 2b99075
  • DEVX-1612 prompt users to rerun wrangler types during dev: 4872cb9

This PR will turn on runtime type generation by default for wrangler types.

The --experimental-include-runtime flag will be superseded by include-runtime and include-env, both of which are true by default.

Also, the runtime types will be generated in the same file as the env types, at the location specified by path (worker-configuration.d.ts by default).

Before:
Screenshot 2025-02-19 at 12 09 27

After:
Screenshot 2025-02-18 at 13 42 47


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • Wrangler E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: covered by unit tests
  • Public documentation

Copy link

changeset-bot bot commented Feb 17, 2025

🦋 Changeset detected

Latest commit: 24d8152

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

Copy link
Contributor

github-actions bot commented Feb 17, 2025

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/13439225900/npm-package-wrangler-8166

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-wrangler-8166 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-workers-bindings-extension-8166 -O ./cloudflare-workers-bindings-extension.0.0.0-va382c8b8b.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-va382c8b8b.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-create-cloudflare-8166 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-kv-asset-handler-8166

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-miniflare-8166

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-pages-shared-8166

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-unenv-preset-8166

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-vite-plugin-8166

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-vitest-pool-workers-8166

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-workers-editor-shared-8166

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-workers-shared-8166

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13439225900/npm-package-cloudflare-workflows-shared-8166

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250204.1
workerd 1.20250214.0 1.20250214.0
workerd --version 1.20250214.0 2025-02-14

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

@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch from 56b09dc to 3d88fea Compare February 17, 2025 18:05
@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch from 3d88fea to 68d6130 Compare February 18, 2025 12:22
@workers-devprod workers-devprod added the e2e Run wrangler e2e tests on a PR label Feb 18, 2025
@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch 3 times, most recently from c86cbf3 to 88426fa Compare February 18, 2025 13:42
@emily-shen emily-shen marked this pull request as ready for review February 18, 2025 13:44
@emily-shen emily-shen requested a review from a team as a code owner February 18, 2025 13:44
@@ -0,0 +1,9 @@
---
"wrangler": minor
Copy link
Contributor Author

@emily-shen emily-shen Feb 18, 2025

Choose a reason for hiding this comment

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

major? pretty sure there's nothing breaking. inclusion in v4 is mostly for politeness as there's a fair amount of CI usage

@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch 4 times, most recently from a7df791 to c931fe0 Compare February 19, 2025 10:31
@emily-shen emily-shen added the v4 wrangler version 4 label Feb 19, 2025
@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch from 9a873ab to 4872cb9 Compare February 20, 2025 14:23

`wrangler types` will now produce one file that contains both `Env` types and runtime types based on your compatibility date and flags. This is located at `worker-configuration.d.ts` by default.

This behaviour was previously gated behind `--experimental-include-runtime`. That flag is no longer necessary and has been removed. It has been replaced by `--include-runtime` and `include-env`, both of which are set to `true` by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This behaviour was previously gated behind `--experimental-include-runtime`. That flag is no longer necessary and has been removed. It has been replaced by `--include-runtime` and `include-env`, both of which are set to `true` by default.
This behaviour was previously gated behind `--experimental-include-runtime`. That flag is no longer necessary and has been removed. It has been replaced by `--include-runtime` and `--include-env`, both of which are set to `true` by default.

@@ -28,7 +28,7 @@ describe("`wrangler types` - file comment", () => {
"packages",
"wrangler"
);
execSync(`npx ${wranglerPath} types ${args}`, {
execSync(`npx ${wranglerPath} types ${args} --include-runtime=false`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the describe l8 be updated? or should we not pass --include-runtime=false ?

Comment on lines +69 to +70
const file = (await readFile(path.join(helper.tmpPath, "./types.d.ts")))
.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can be updated at multiple places

Suggested change
const file = (await readFile(path.join(helper.tmpPath, "./types.d.ts")))
.toString()
const file = (await readFile(path.join(helper.tmpPath, "./types.d.ts"), "utf8"))

`"// Generated by Wrangler by running \`wrangler types ./types.d.ts\` (hash: e82ba4d7b995dd9ca6fb0332d81f889b)"`
);
expect(file[1]).match(
/\/\/ Runtime types generated with workerd@1\.\d+\.\d \d\d\d\d-\d\d-\d\d ([a-z_]+,?)*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/\/\/ Runtime types generated with workerd@1\.\d+\.\d \d\d\d\d-\d\d-\d\d ([a-z_]+,?)*/
/\/\/ Runtime types generated with workerd@1\.\d+\.\d \d+-\d+-\d+ ([a-z_]+,?)*/

or \d{4} / \d{2} but + is consistent with the first d+

const output = await helper.run(
`wrangler types --x-include-runtime="./types.d.ts"`
);
const file2 = (await readFile(typesPath)).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

utf8

if (!entry.file.endsWith(".ts")) {
return;
}
let maybeExistingTypesFile: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let maybeExistingTypesFile: string[];
let maybeExistingTypesFileLines: string[];

import type { Config } from "../config";
import type { Entry } from "../deployment-bundle/entry";

export const checkTypesDiff = async (config: Config, entry: Entry) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about what this fn does?

try {
const { envHeader } = await generateEnvTypes(
config,
{ strictVars: previousStrictVars === "false" ? false : true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ strictVars: previousStrictVars === "false" ? false : true },
{ strictVars: previousStrictVars !== "false" },

Comment on lines +57 to +58
`Since you have the \`nodejs_compat\` flag, you should install Node.js types by running "npm i --save-dev @types/node${isWorkersTypesInstalled ? '@20.8.3".\nFor more details: https://developers.cloudflare.com/workers/languages/typescript/#known-issues' : '".'}`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like the following might improve readibility?

Suggested change
`Since you have the \`nodejs_compat\` flag, you should install Node.js types by running "npm i --save-dev @types/node${isWorkersTypesInstalled ? '@20.8.3".\nFor more details: https://developers.cloudflare.com/workers/languages/typescript/#known-issues' : '".'}`
)
`Since you have the \`nodejs_compat\` flag, you should install Node.js types by running "npm i --save-dev ${isWorkersTypesInstalled ? "@types/node@20.8.3.\nFor more details: https://developers.cloudflare.com/workers/languages/typescript/#known-issues' : '@types/node.'}`
)

@emily-shen emily-shen changed the base branch from main to next February 20, 2025 15:22
@emily-shen emily-shen changed the base branch from next to main February 20, 2025 15:23
@emily-shen emily-shen force-pushed the emily/dynamic-typegen-ga branch from 4872cb9 to 2ef9aa4 Compare February 20, 2025 15:24
@emily-shen emily-shen requested review from a team as code owners February 20, 2025 15:24
@emily-shen emily-shen changed the base branch from main to next February 20, 2025 15:24

try {
const existingTypes = await readFile(outFile, "utf8");
if (existingTypes.split("\n")[0] === header) {
const file = (await readFile(outFile, "utf8")).split("\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const file = (await readFile(outFile, "utf8")).split("\n");
const lines = (await readFile(outFile, "utf8")).split("\n");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c3-e2e e2e Run wrangler e2e tests on a PR v4 wrangler version 4
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

3 participants