-
Notifications
You must be signed in to change notification settings - Fork 553
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 file support in pages dev
#5284
Conversation
🦋 Changeset detectedLatest commit: 77f8fb0 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 |
4205ca5
to
317e60f
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/8437248250/npm-package-wrangler-5284 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5284/npm-package-wrangler-5284 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-wrangler-5284 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-create-cloudflare-5284 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-cloudflare-kv-asset-handler-5284 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-miniflare-5284 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-cloudflare-pages-shared-5284 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8437248250/npm-package-cloudflare-vitest-pool-workers-5284 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
317e60f
to
a07461a
Compare
pages dev
pages dev
cf3db9b
to
fef36bf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5284 +/- ##
==========================================
- Coverage 72.39% 72.35% -0.04%
==========================================
Files 319 320 +1
Lines 16534 16579 +45
Branches 4236 4246 +10
==========================================
+ Hits 11970 11996 +26
- Misses 4564 4583 +19
|
/** | ||
* | ||
*/ | ||
function resolvePagesDevServerSettings( |
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.
in need of a better naming here 🤔 I think. Maybe reconcilePagesDevServerSettings
or mergePagesDevServerSettings
? Or maybe this is fine as it is?
pages dev
pages dev
fef36bf
to
2735f94
Compare
5c0a4fb
to
079589a
Compare
return hyperdrive; | ||
}); | ||
|
||
// Queues bindings ?? |
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.
//todo @CarmenPopoviciu dbl check this is correct
e340ab0
to
26af7ad
Compare
26af7ad
to
460f638
Compare
Regarding this test case:
Should we consider outputting a warning to the user, in case they were actually trying to create a toml for Pages and just missed that field? |
One slight issue here is that this represents a break when compared to existing behaviour (where currently |
Is this really a breaking change tho? ...given that current behaviour is that Pages does not support |
I think |
I see...I was not aware of that. Any thoughts on this Dario? |
@CarmenPopoviciu yes, @penalosa is right this would be a bit breaking for I mean not the utility itself obviously but the workflow with it and I think it would indeed be beneficial if we could not include this mandatory field for now and just show a warning regarding it until make the next major version of wrangler, giving users and frameworks some time to adopt/migrate over 🤔
We recently did make this contract public: https://developers.cloudflare.com/pages/functions/bindings/ (cloudflare/cloudflare-docs#12731) So I would indeed think that this would be a breaking change regardless of |
33dc369
to
c5358bf
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.
Couple nits (and the change from ignoring to warning on invalid config files) but otherwise LGTM!
After talking abt this internally, we're going to skip any warning for this use case, and keep the current behaviour as it is. I added a test case in my test plan to reflect this behaviour. Pls see:
|
be40898
to
5b06f12
Compare
As we are adding `wrangler.toml` support for Pages, we want to ensure that `wrangler pages dev` works with a configuration file. This commit adds `wrangler.toml` support for local development using `pages dev`.
5b06f12
to
77f8fb0
Compare
fix(wrangler): fix `pages dev` default port #5284 changed the default port used by `wrangler pages dev` from `8788` to `8787`. Unfortunately this is a regression, as some folks rely on the previous port number. This commit reverts the change, and sets the default port back to `8788`.
What this PR solves / how to test
This PR adds
wrangler.toml
support inpages dev
♫♫♫
♫♫♫
Author has addressed the following
Test Plan 🚀
Apart from unit/integration/fixtures and e2e tests, this PR was manually tested based on the following test plan:
pages dev
should pick up configuration fromwrangler.toml
, if file exists andpages_build_output_dir
is specified in the configurationpages dev
should fail ifwrangler.toml
exists butpages_build_output_dir
is not specified in the configurationpages dev
should fail ifwrangler.toml
file is not validpages dev
should apply top level non environment config specified inwrangler.toml
pages dev
should apply top level local development config specified inwrangler.toml
=== terminal ===
=== browser ===
=== dev-tools ===
pages dev
should apply top level inheritable environment config specified inwrangler.toml
pages dev
should apply top level non-inheritable environment config specified inwrangler.toml
pages dev
should merge cmd line args with config fromwrangler.toml
wrangler.toml
and as a command arg topages dev
, the arg value should take precedence and override the value specified inwrangler.toml
pages dev
should not apply any environment (preview
|production
) configuration, if specified. It should always defer to the top level config.pages dev <directory>
should pick upwrangler.toml
configuration, even ifpages_build_output_dir
is not specified