Skip to content
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

Allow setting d1 database id in pages dev #3485

Merged
merged 6 commits into from
Jun 20, 2023

Conversation

GregBrimble
Copy link
Member

@GregBrimble GregBrimble commented Jun 17, 2023

What this PR solves / how to test:

Allows setting a D1 database ID when using wrangler pages dev by providing an optional =<ID> suffix to the argument like --d1 BINDING_NAME=database-id.

This is useful when a Worker and a Pages project are sharing local dev state (--persist-to) location.

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@GregBrimble GregBrimble requested review from a team as code owners June 17, 2023 16:01
@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2023

🦋 Changeset detected

Latest commit: d36687e

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

This PR includes changesets to release 1 package
Name Type
wrangler 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
Copy link
Contributor

github-actions bot commented Jun 17, 2023

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/5322823523/npm-package-wrangler-3485

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5322823523/npm-package-wrangler-3485 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5322823523/npm-package-cloudflare-pages-shared-3485

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

@GregBrimble GregBrimble force-pushed the allow-pages-dev-set-d1-database-id branch from c45d4a3 to 8f36018 Compare June 17, 2023 16:03
@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #3485 (d36687e) into main (fcaaa9f) will decrease coverage by 0.10%.
The diff coverage is 4.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3485      +/-   ##
==========================================
- Coverage   75.18%   75.08%   -0.10%     
==========================================
  Files         183      183              
  Lines       11055    11075      +20     
  Branches     2904     2916      +12     
==========================================
+ Hits         8312     8316       +4     
- Misses       2743     2759      +16     
Impacted Files Coverage Δ
packages/wrangler/src/pages/dev.ts 16.22% <4.16%> (-0.92%) ⬇️

... and 2 files with indirect coverage changes

@GregBrimble GregBrimble force-pushed the allow-pages-dev-set-d1-database-id branch from 8f36018 to 6349a16 Compare June 17, 2023 16:24
Copy link
Contributor

@Cherry Cherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems useful until a config file is officially supported, but perhaps would make more sense extending to other bindings too generically, so they share a similar syntax? R2, KV, etc.

Otherwise D1 having this own specific syntax is a little strange.

@GregBrimble GregBrimble force-pushed the allow-pages-dev-set-d1-database-id branch 2 times, most recently from 009a2b4 to 2debdc9 Compare June 17, 2023 17:38
@@ -32,6 +32,8 @@ const DURABLE_OBJECTS_BINDING_REGEXP = new RegExp(
/^(?<binding>[^=]+)=(?<className>[^@\s]+)(@(?<scriptName>.*)$)?$/
);

const BINDING_REGEXP = new RegExp(/^(?<binding>[^=]+)(?:=(?<ref>[^\s]+))?$/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) Is the new RegExp needed? Seeing as the regex is composed syntactically

@GregBrimble GregBrimble force-pushed the allow-pages-dev-set-d1-database-id branch from 828a085 to db5560f Compare June 19, 2023 15:27
@GregBrimble GregBrimble force-pushed the allow-pages-dev-set-d1-database-id branch from d1c625a to bfe1d02 Compare June 19, 2023 15:50
@GregBrimble
Copy link
Member Author

GregBrimble commented Jun 19, 2023

Rebased so fixture tests actually run after #3489 🙃

@GregBrimble GregBrimble merged commit e8df68e into main Jun 20, 2023
@GregBrimble GregBrimble deleted the allow-pages-dev-set-d1-database-id branch June 20, 2023 18:27
@github-actions github-actions bot mentioned this pull request Jun 20, 2023
lrapoport-cf pushed a commit that referenced this pull request Aug 11, 2023
* Allow setting d1 database id in pages dev

* Allow KV and R2 to be referenced to too

* Fix state directory in fixture test

* Add comments describing the binding regexps

* Remove duplicate import

* More defensively parse bindings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants