-
Notifications
You must be signed in to change notification settings - Fork 688
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
Config controller #6001
Config controller #6001
Conversation
🦋 Changeset detectedLatest commit: 1cef88e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
2c2c4e8
to
5ecfc9a
Compare
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 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.
Please ensure constraints are pinned, and |
This comment was marked as spam.
This comment was marked as spam.
name: "worker-name", | ||
}, | ||
_additionalModules: [], | ||
script: { path: path.resolve("src/index.ts") }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
3b60bec
to
5b1e416
Compare
5b1e416
to
ea50f1d
Compare
3847e22
to
a19fd8f
Compare
2456833
to
1cef88e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few tiny nits, but nothing to hold it back.
}, | ||
_additionalModules: [], | ||
entrypoint: { path: path.resolve("src/index.ts") }, | ||
directory: path.resolve("src"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, directory
has some other meaning than just "parent directory of entrypoint
"?
assert(config.entrypoint.path); | ||
assert(config.directory); | ||
assert(config.build?.format); | ||
assert(config.build?.moduleRoot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love seeing these assert
s everywhere.
cwd: config.build?.custom.workingDirectory, | ||
command: config.build?.custom.command, | ||
await runCustomBuild(config.entrypoint.path, relativeFile, { | ||
cwd: config.build?.custom?.workingDirectory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - let's see how it flies!
devEnv.proxy.onBundleStart({ | ||
type: "bundleStart", | ||
config: startDevWorkerOptions, | ||
}); | ||
}, [devEnv, startDevWorkerOptions]); | ||
}, [devEnv, startDevWorkerOptions, props.experimentalDevEnv]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here #6116
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