Config controller #6001

penalosa · 2024-06-10T16:17:45Z

What this PR solves / how to test

This change removes some 'passthrough' config options from the BundleController PR and now uses expected StartDevWorkerOptions.

A thin implementation of the config controller has been implemented and integrated into the react-flow behind the existing -x-dev-env flag.

This change is covered by local+remote e2e tests both with+without the –x-dev-env flag. The dev-env fixture tests will be updated in a follow-up PR.

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: refactor behind experimental flag
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: refactor behind experimental flag

changeset-bot · 2024-06-10T16:17:56Z

🦋 Changeset detected

Latest commit: 1cef88e

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@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

github-actions · 2024-06-10T17:36:29Z

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/9596579703/npm-package-wrangler-6001

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9596579703/npm-package-wrangler-6001 dev path/to/script.js

Additional artifacts:

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

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

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

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

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

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

wrangler@3.61.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240610.1
`workerd`	1.20240610.1	1.20240610.1
`workerd --version`	1.20240610.1	2024-06-10

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

RamIdeas · 2024-06-12T14:36:27Z

packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts

-				name: "worker-name",
-			},
-			_additionalModules: [],
+			script: { path: path.resolve("src/index.ts") },


As a user I would expect this filepath to be relative to the cwd. So the user shouldn't have to call path.resolve – the ConfigController should do that on their behalf

packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts

packages/wrangler/src/api/startDevWorker/BundlerController.ts

packages/wrangler/src/dev/dev.tsx

packages/wrangler/src/api/startDevWorker/types.ts

packages/wrangler/src/dev.tsx

packages/wrangler/src/dev/dev.tsx

packages/wrangler/src/api/startDevWorker/BundlerController.ts

andyjessop

Looks great! A few tiny nits, but nothing to hold it back.

andyjessop · 2024-06-20T11:48:29Z

packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts

-			},
-			_additionalModules: [],
+			entrypoint: { path: path.resolve("src/index.ts") },
+			directory: path.resolve("src"),


Presumably, directory has some other meaning than just "parent directory of entrypoint"?

andyjessop · 2024-06-20T11:49:35Z

packages/wrangler/src/api/startDevWorker/BundlerController.ts

+		assert(config.entrypoint.path);
+		assert(config.directory);
+		assert(config.build?.format);
+		assert(config.build?.moduleRoot);


I love seeing these asserts everywhere.

andyjessop · 2024-06-20T11:53:29Z

packages/wrangler/src/api/startDevWorker/BundlerController.ts

-				cwd: config.build?.custom.workingDirectory,
-				command: config.build?.custom.command,
+			await runCustomBuild(config.entrypoint.path, relativeFile, {
+				cwd: config.build?.custom?.workingDirectory,


Is it correct that we can allow an undefined working directory here? I think it will default to the directory that the node process was launched from.

petebacondarwin

LGTM - let's see how it flies!

threepointone · 2024-06-20T21:43:32Z

packages/wrangler/src/dev/dev.tsx

 		devEnv.proxy.onBundleStart({
 			type: "bundleStart",
 			config: startDevWorkerOptions,
 		});
-	}, [devEnv, startDevWorkerOptions]);
+	}, [devEnv, startDevWorkerOptions, props.experimentalDevEnv]);


@RamIdeas @penalosa What is the expected behaviour here? I think your comment was incomplete, so I don't understand this. Did you mean to write if(props.experimentalDevEnv) { devEnv.proxy.onBundleStart(/*...*/) }

Did you mean to write if(props.experimentalDevEnv) { devEnv.proxy.onBundleStart(/.../) }

No, explicitly the other way around. The faking of events is what wrangler main was doing. The experimental flag is newer and is the code path of real events within DevEnv.

Slightly confusing, so I put an if statement around it if(!props.experimentalDevEnv) { devEnv.proxy.onBundleStart(/*...*/) } (note the !) to be explicit but Samuel and I then then decided it was unnecessary as onBundleStart is not called unless props.experimentalDevEnv === false. So we settled on a comment instead. The prop left in the dependency array was a mistake but is not an issue since it is static and won't change. I'll make a follow-up PR to remove the extraneous dependency array item.

Actually I'm going to put the if statement back just to be super explicit and avoid confusion. Even if it's redundant, it being code and not a comment will avoid confusion as to whether it is correct.

Addressed here #6116

github-actions bot added the e2e Run e2e tests on a PR label Jun 10, 2024

penalosa added 6 commits June 10, 2024 18:32

Remove entry passthrough

d1ef9ef

Remove _projectRoot passthrough

96c2fb4

Refactor site/assets/legacy env

d0027f4

Remove _serveAssetsFromWorker

6dd781b

additionalModules

25e59ad

no more passthrough

5ecfc9a

penalosa force-pushed the config-controller branch from 2c2c4e8 to 5ecfc9a Compare June 10, 2024 17:33

remove _bindings

a1a75b1

This comment was marked as spam.

Sign in to view

penalosa added 4 commits June 10, 2024 23:16

fix tests

54ebb45

fix test types

9841566

types

0a6ce02

wip

2da28da

RamIdeas reviewed Jun 12, 2024

View reviewed changes

packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 12, 2024

View reviewed changes

packages/wrangler/src/api/startDevWorker/BundlerController.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 12, 2024

View reviewed changes

packages/wrangler/src/api/startDevWorker/BundlerController.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/dev/dev.tsx Show resolved Hide resolved

RamIdeas and others added 4 commits June 18, 2024 12:48

fix: emit-types

bb8f8df

fix: return statement inside->outside loop

615c683

wip

0c1cb7b

Merge branch 'main' into config-controller

fbdc5a3

penalosa commented Jun 18, 2024

View reviewed changes

packages/wrangler/src/api/startDevWorker/types.ts Show resolved Hide resolved

penalosa and others added 4 commits June 18, 2024 17:24

fix lint

0a6ec99

Merge remote-tracking branch 'origin/main' into config-controller

f22d3cb

Merge branch 'main' into config-controller

f180380

avoid StartDevWorkerOptions mutation

fd900d0

chore: use of nullish config properties

2bd5374

RamIdeas marked this pull request as ready for review June 19, 2024 18:11

RamIdeas requested a review from a team as a code owner June 19, 2024 18:11

RamIdeas force-pushed the config-controller branch from 3b60bec to 5b1e416 Compare June 19, 2024 18:54

rename: script to entrypoint

ea50f1d

RamIdeas force-pushed the config-controller branch from 5b1e416 to ea50f1d Compare June 19, 2024 21:05

RamIdeas mentioned this pull request Jun 19, 2024

Support --x-dev-env for Pages #6097

Open

12 tasks

RamIdeas added 2 commits June 20, 2024 11:57

remove temp change

c09bcdd

add changeset

a19fd8f

RamIdeas force-pushed the config-controller branch from 3847e22 to a19fd8f Compare June 20, 2024 11:03

penalosa commented Jun 20, 2024

View reviewed changes

packages/wrangler/src/dev.tsx Outdated Show resolved Hide resolved

packages/wrangler/src/dev/dev.tsx Outdated Show resolved Hide resolved

packages/wrangler/src/api/startDevWorker/BundlerController.ts Outdated Show resolved Hide resolved

address comments

1cef88e

RamIdeas force-pushed the config-controller branch from 2456833 to 1cef88e Compare June 20, 2024 11:24

andyjessop approved these changes Jun 20, 2024

View reviewed changes

petebacondarwin approved these changes Jun 20, 2024

View reviewed changes

RamIdeas merged commit d39d595 into main Jun 20, 2024
21 checks passed

RamIdeas deleted the config-controller branch June 20, 2024 13:48

workers-devprod mentioned this pull request Jun 20, 2024

Version Packages #6081

Merged

threepointone reviewed Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config controller #6001

Config controller #6001

penalosa commented Jun 10, 2024 •

edited by RamIdeas

Loading

changeset-bot bot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

This comment was marked as spam.

RamIdeas Jun 12, 2024

andyjessop left a comment

andyjessop Jun 20, 2024

andyjessop Jun 20, 2024

andyjessop Jun 20, 2024

petebacondarwin left a comment

threepointone Jun 20, 2024 •

edited

Loading

RamIdeas Jun 21, 2024

RamIdeas Jun 21, 2024

RamIdeas Jun 21, 2024

Config controller #6001

Config controller #6001

Conversation

penalosa commented Jun 10, 2024 • edited by RamIdeas Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jun 10, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 10, 2024 • edited Loading

This comment was marked as spam.

RamIdeas Jun 12, 2024

Choose a reason for hiding this comment

andyjessop left a comment

Choose a reason for hiding this comment

andyjessop Jun 20, 2024

Choose a reason for hiding this comment

andyjessop Jun 20, 2024

Choose a reason for hiding this comment

andyjessop Jun 20, 2024

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

threepointone Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

RamIdeas Jun 21, 2024

Choose a reason for hiding this comment

RamIdeas Jun 21, 2024

Choose a reason for hiding this comment

RamIdeas Jun 21, 2024

Choose a reason for hiding this comment

penalosa commented Jun 10, 2024 •

edited by RamIdeas

Loading

changeset-bot bot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

threepointone Jun 20, 2024 •

edited

Loading