Config file support in `pages dev` #5284

CarmenPopoviciu · 2024-03-18T23:13:16Z

What this PR solves / how to test

This PR adds wrangler.toml support in pages dev

♫♫♫

Everybody spread the word
We're gonna have a celebration
All across the world
In every nation
It's time for the good times
Forget about the bad times, oh yeah
One day to come together to release the pressure
We need a holiday

♫♫♫

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
- Not necessary because: not required at this stage of the work

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 from wrangler.toml, if file exists and pages_build_output_dir is specified in the configuration

npx wrangler pages dev

pages dev should fail if wrangler.toml exists but pages_build_output_dir is not specified in the configuration

npx wrangler pages dev

pages dev should fail if wrangler.toml file is not valid

# wrangler.toml

[env.staging]
compatibility_date = "2024-01-01"

npx wrangler pages dev

pages dev should apply top level non environment config specified in wrangler.toml
pages dev should apply top level local development config specified in wrangler.toml

# wrangler.toml

[dev]
ip = "127.0.0.1"
port = 8181
inspector_port = 8989
local_protocol = "http"
upstream_protocol = "http"

npx wrangler pages dev

=== terminal ===

=== browser ===

=== dev-tools ===

pages dev should apply top level inheritable environment config specified in wrangler.toml

# wrangler.toml

# test by commenting out `compatibility_date`
#compatibility_date = "2024-01-01"

npx wrangler pages dev

pages dev should apply top level non-inheritable environment config specified in wrangler.toml

npx wrangler pages dev

pages dev should merge cmd line args with config from wrangler.toml

npx wrangler pages dev 
  --kv kv_binding_args 
  --binding VAR3="var3" 
  --do do_binding_args=DO@a 
  --d1 d1_binding_args 
  --r2 r2_binding_args 
  --ai ai_binding_args 
  --service service_binding_args=worker-args

when a config field is specified in both wrangler.toml and as a command arg to pages dev, the arg value should take precedence and override the value specified in wrangler.toml

npx wrangler pages dev
  --kv KV_BINDING_TOML=new_kv_id 
  --binding VAR1="new_var_1" 
  --do DO_BINDING_TOML=new_do@new_do_script 
  --d1 D1_BINDING_TOML=new_d1_binding 
  --r2 R2_BINDING_TOML=new_r2_binding 
  --ai ai_binding_args 
  --service SERVICE_BINDING_TOML=new_service_name

pages dev should not apply any environment (preview | production) configuration, if specified. It should always defer to the top level config.

# wrangler.toml

## Preview Environment
[env.preview]
[[env.preview.kv_namespaces]]
binding = "KV_BINDING_PREVIEW_TOML"
id = "KV_ID_PREVIEW_TOML"

## Production Environment
[env.production]
[[env.production.kv_namespaces]]
binding = "KV_BINDING_PRODUCTION_TOML"
id = "KV_ID_PRODUCTION_TOML"

npx wrangler pages dev

pages dev <directory> should pick up wrangler.toml configuration, even if pages_build_output_dir is not specified

# wrangler.toml
name = "pages-project"
compatibility_date = "2024-01-01"

[vars]
VAR1 = "holiday"
VAR2 = "celebrate"

npx wrangler pages dev public

changeset-bot · 2024-03-18T23:13:20Z

🦋 Changeset detected

Latest commit: 77f8fb0

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

github-actions · 2024-03-19T18:32:09Z

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 with this latest build directly:

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.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240320.0
`workerd`	1.20240320.1	1.20240320.1
`workerd --version`	1.20240320.1	2024-03-20

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

codecov · 2024-03-19T22:05:54Z

Codecov Report

Attention: Patch coverage is 29.22078% with 109 lines in your changes are missing coverage. Please review.

Project coverage is 72.35%. Comparing base (ba52208) to head (77f8fb0).
Report is 1 commits behind head on main.

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

Files	Coverage Δ
packages/wrangler/src/config/validation-pages.ts	`97.67% <100.00%> (+0.05%)`	⬆️
packages/wrangler/src/utils/mergeWithOverride.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/dev.tsx	`80.98% <44.89%> (+1.06%)`	⬆️
packages/wrangler/src/pages/dev.ts	`32.12% <15.46%> (-1.00%)`	⬇️

... and 5 files with indirect coverage changes

CarmenPopoviciu · 2024-03-20T08:51:36Z

packages/wrangler/src/pages/dev.ts

+/**
+ *
+ */
+function resolvePagesDevServerSettings(


in need of a better naming here 🤔 I think. Maybe reconcilePagesDevServerSettings or mergePagesDevServerSettings? Or maybe this is fine as it is?

CarmenPopoviciu · 2024-03-24T21:27:34Z

packages/wrangler/src/dev.tsx

+		return hyperdrive;
+	});
+
+	// Queues bindings ??


//todo @CarmenPopoviciu dbl check this is correct

packages/wrangler/src/dev.tsx

packages/wrangler/e2e/pages-dev.test.ts

packages/wrangler/src/dev.tsx

packages/wrangler/src/pages/dev.ts

petebacondarwin · 2024-03-25T09:50:04Z

Regarding this test case:

pages dev should ignore wrangler.toml, if it exists but pages_build_output_dir is not specified in the configuration

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?

penalosa · 2024-03-25T12:03:56Z

Regarding this test case:

* pages dev should ignore wrangler.toml, if it exists but `pages_build_output_dir` is not specified in the configuration

One slight issue here is that this represents a break when compared to existing behaviour (where currently pages dev unconditionally reads wrangler.toml). I wonder if we want to have a brief period where pages dev supports a config file without pages_build_output_dir, but warns loudly that the behaviour is about to change?

CarmenPopoviciu · 2024-03-25T12:20:20Z

Regarding this test case:
* pages dev should ignore wrangler.toml, if it exists but `pages_build_output_dir` is not specified in the configuration
One slight issue here is that this represents a break when compared to existing behaviour (where currently pages dev unconditionally reads wrangler.toml). I wonder if we want to have a brief period where pages dev supports a config file without pages_build_output_dir, but warns loudly that the behaviour is about to change?

Is this really a breaking change tho? ...given that current behaviour is that Pages does not support wrangler.toml, and the reason why pages dev works with wrangler.toml is purely coincidental. There is nowhere, that I am aware of at least, where we made this contract public

penalosa · 2024-03-25T12:33:37Z

Regarding this test case:
* pages dev should ignore wrangler.toml, if it exists but `pages_build_output_dir` is not specified in the configuration
One slight issue here is that this represents a break when compared to existing behaviour (where currently pages dev unconditionally reads wrangler.toml). I wonder if we want to have a brief period where pages dev supports a config file without pages_build_output_dir, but warns loudly that the behaviour is about to change?
Is this really a breaking change tho? ...given that current behaviour is that Pages does not support wrangler.toml, and the reason why pages dev works with wrangler.toml is purely coincidental. There is nowhere, that I am aware of at least, where we made this contract public

I think getBindingsProxy() and a lot of the recent frameworks work depends on it, but I'll defer to @dario-piotrowicz

CarmenPopoviciu · 2024-03-25T12:42:26Z

I see...I was not aware of that. Any thoughts on this Dario?

dario-piotrowicz · 2024-03-25T13:19:30Z

@CarmenPopoviciu yes, @penalosa is right this would be a bit breaking for getPlatformProxy...

I mean not the utility itself obviously but the workflow with it and wrangler pages dev 😕

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 🤔

Is this really a breaking change tho? ...given that current behaviour is that Pages does not support wrangler.toml, and the reason why pages dev works with wrangler.toml is purely coincidental. There is nowhere, that I am aware of at least, where we made this contract public

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 getPlatformProxy, no? 🤔 (cc. @admah)

penalosa

Couple nits (and the change from ignoring to warning on invalid config files) but otherwise LGTM!

packages/wrangler/e2e/pages-dev.test.ts

packages/wrangler/src/dev.tsx

packages/wrangler/src/pages/dev.ts

CarmenPopoviciu · 2024-03-25T17:17:26Z

Regarding this test case:
* pages dev should ignore wrangler.toml, if it exists but `pages_build_output_dir` is not specified in the configuration
One slight issue here is that this represents a break when compared to existing behaviour (where currently pages dev unconditionally reads wrangler.toml). I wonder if we want to have a brief period where pages dev supports a config file without pages_build_output_dir, but warns loudly that the behaviour is about to change?
Is this really a breaking change tho? ...given that current behaviour is that Pages does not support wrangler.toml, and the reason why pages dev works with wrangler.toml is purely coincidental. There is nowhere, that I am aware of at least, where we made this contract public

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:

pages dev <directory> should pick up wrangler.toml configuration, even if pages_build_output_dir is not specified

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`.

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`.

CarmenPopoviciu requested review from a team as code owners March 18, 2024 23:13

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch 2 times, most recently from 4205ca5 to 317e60f Compare March 19, 2024 18:29

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from 317e60f to a07461a Compare March 19, 2024 21:27

CarmenPopoviciu changed the title ~~[WIP] Config file support for pages dev~~ [WIP] Config file support in pages dev Mar 19, 2024

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch 2 times, most recently from cf3db9b to fef36bf Compare March 19, 2024 21:55

CarmenPopoviciu commented Mar 20, 2024

View reviewed changes

CarmenPopoviciu changed the title ~~[WIP] Config file support in pages dev~~ Config file support in pages dev Mar 24, 2024

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from fef36bf to 2735f94 Compare March 24, 2024 21:22

CarmenPopoviciu requested a review from a team as a code owner March 24, 2024 21:22

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch 3 times, most recently from 5c0a4fb to 079589a Compare March 24, 2024 21:25

CarmenPopoviciu commented Mar 24, 2024

View reviewed changes

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

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch 2 times, most recently from e340ab0 to 26af7ad Compare March 24, 2024 21:43

CarmenPopoviciu added the e2e Run wrangler + vite-plugin e2e tests on a PR label Mar 24, 2024

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from 26af7ad to 460f638 Compare March 24, 2024 22:05

petebacondarwin reviewed Mar 25, 2024

View reviewed changes

petebacondarwin added the ldw-exempt PRs marked with this label can be included in releases during LDW. label Mar 25, 2024

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from 33dc369 to c5358bf Compare March 25, 2024 13:46

penalosa approved these changes Mar 25, 2024

View reviewed changes

penalosa mentioned this pull request Mar 25, 2024

🐛 BUG: wrangler pages dev cannot detect compatibility_date from wrangler.toml #4411

Closed

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from be40898 to 5b06f12 Compare March 25, 2024 17:19

petebacondarwin approved these changes Mar 26, 2024

View reviewed changes

feat(wrangler): Add wrangler.toml support in pages dev

77f8fb0

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`.

CarmenPopoviciu force-pushed the carmen/pages-config-dev branch from 5b06f12 to 77f8fb0 Compare March 26, 2024 14:00

CarmenPopoviciu merged commit f5e2367 into main Mar 26, 2024

CarmenPopoviciu deleted the carmen/pages-config-dev branch March 26, 2024 15:21

workers-devprod mentioned this pull request Mar 26, 2024

Version Packages #5358

Merged

Config file support in pages dev #5284

Config file support in pages dev #5284

Uh oh!

Conversation

CarmenPopoviciu commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR solves / how to test

Author has addressed the following

Test Plan 🚀

Uh oh!

changeset-bot bot commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CarmenPopoviciu Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu Mar 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Mar 25, 2024

Uh oh!

penalosa commented Mar 25, 2024

Uh oh!

CarmenPopoviciu commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penalosa commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarmenPopoviciu commented Mar 25, 2024

Uh oh!

dario-piotrowicz commented Mar 25, 2024

Uh oh!

penalosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CarmenPopoviciu commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Config file support in `pages dev` #5284

Config file support in `pages dev` #5284

CarmenPopoviciu commented Mar 18, 2024 •

edited

Loading

changeset-bot bot commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

CarmenPopoviciu Mar 20, 2024 •

edited

Loading

CarmenPopoviciu commented Mar 25, 2024 •

edited

Loading

penalosa commented Mar 25, 2024 •

edited

Loading

CarmenPopoviciu commented Mar 25, 2024 •

edited

Loading