-
Notifications
You must be signed in to change notification settings - Fork 692
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(pages): wrangler pages dev
should prioritize _worker.js
#1950
Conversation
🦋 Changeset detectedLatest commit: 3c0efbf 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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/3181524686/npm-package-wrangler-1950 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1950/npm-package-wrangler-1950 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/3181524686/npm-package-wrangler-1950 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.developers.workers.dev/runs/3181524686/npm-package-cloudflare-pages-shared-1950 |
Codecov Report
@@ Coverage Diff @@
## main #1950 +/- ##
==========================================
- Coverage 72.72% 72.70% -0.02%
==========================================
Files 118 118
Lines 8230 8229 -1
Branches 2160 2160
==========================================
- Hits 5985 5983 -2
- Misses 2245 2246 +1
|
637cd3d
to
23a294e
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.
LGTM
|
||
if (url.pathname.startsWith("/date")) { | ||
return new Response( | ||
"Yesterday is history, tomorrow is a mystery, but today is a gift. That’s why it is called the present." |
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.
🙇
}; | ||
|
||
await runBuild(); | ||
watch([scriptPath], { |
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.
Does this watch only get cancelled if you kill the process?
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.
@petebacondarwin that is correct. In fact that is true for all our watch
ers. I am rather new to watch
and esbuild
so my reasoning might be wrong, but the way I see it is that we want to keep watching these files (_worker.js
, /functions/*
and _routes.json
) for changes for as long as the process is running, so we can serve the right assets.
Is there a use case you had in mind that would require us to close the watcher? Or what was the underlying concern of your question?
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.
My only thought was whether esbuild kicks off a child process that might somehow get orphaned if we closed down the process in some other way than killing it? But I think that is probably not the case.
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.
according to evanw/esbuild#643 this 👉 https://github.com/evanw/esbuild/blob/master/cmd/esbuild/service.go#L124-L137 should prevent something like that from happening. So I think we should be OK
When using a _worker.js file, the entire /functions directory should be ignored – this includes its routing and middleware characteristics. Currently `wrangler pages dev` does the reverse, by prioritizing `/functions` over `_worker.js`. This commit fixes that.
23a294e
to
3c0efbf
Compare
When using a _worker.js file, the entire
/functions
directory should be ignored – this includes its routing and middleware characteristics. Currentlywrangler pages dev
does the reverse, by prioritizing/functions
over_worker.js
. This commit fixes that.