Skip to content

improve and test vite-plugin-cloudflare .dev.vars files support#7864

Merged
dario-piotrowicz merged 12 commits intomainfrom
dario/7850/dev-vars-in-vite-plugin
Jan 28, 2025
Merged

improve and test vite-plugin-cloudflare .dev.vars files support#7864
dario-piotrowicz merged 12 commits intomainfrom
dario/7850/dev-vars-in-vite-plugin

Conversation

@dario-piotrowicz
Copy link
Copy Markdown
Member

@dario-piotrowicz dario-piotrowicz commented Jan 22, 2025

fixes #7850

the changes here add tests for making sure that the vite plugin correctly reads secrets from .dev.vars files with and without a specified environment

as part of this wragler's unstable_getMiniflareWorkerOptions utility needed to also be updated to accept the environment name as its second parameter


  • Tests
    • TODO (before merge)
    • Tests included (tests added for the vite-plugin, as for wrangler, it looks like the unstable_getMiniflareWorkerOptions utility is not currently tested, so I don't think it's necessary to include any here either)
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: documentation added as part of our vite-plugin README (which is currently the only documentation the package has)

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 22, 2025

🦋 Changeset detected

Latest commit: a39093d

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

This PR includes changesets to release 3 packages
Name Type
@cloudflare/vite-plugin Patch
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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 22, 2025

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/13001188073/npm-package-wrangler-7864

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-wrangler-7864 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-workers-bindings-extension-7864 -O ./cloudflare-workers-bindings-extension.0.0.0-vc2ba65d44.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vc2ba65d44.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-create-cloudflare-7864 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-kv-asset-handler-7864

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-miniflare-7864

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-pages-shared-7864

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-unenv-preset-7864

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-vite-plugin-7864

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-vitest-pool-workers-7864

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-workers-editor-shared-7864

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-workers-shared-7864

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13001188073/npm-package-cloudflare-workflows-shared-7864

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


wrangler@3.105.1 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250124.0
workerd 1.20250124.0 1.20250124.0
workerd --version 1.20250124.0 2025-01-24

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

@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from e3e9d3a to 69affde Compare January 22, 2025 15:01
@dario-piotrowicz dario-piotrowicz added vite-plugin Relating to the `@cloudflare/vite-plugin` package e2e Run wrangler + vite-plugin e2e tests on a PR labels Jan 22, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch 3 times, most recently from b1407a5 to 6e539ad Compare January 22, 2025 17:21
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review January 22, 2025 17:24
@dario-piotrowicz dario-piotrowicz requested a review from a team as a code owner January 22, 2025 17:24
@dario-piotrowicz dario-piotrowicz requested a review from a team January 22, 2025 17:24
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
@dario-piotrowicz dario-piotrowicz requested a review from vicb January 22, 2025 18:40
@dario-piotrowicz dario-piotrowicz marked this pull request as draft January 23, 2025 11:32
Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from dbe3e1b to 8689cad Compare January 23, 2025 14:07
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review January 23, 2025 14:22
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from 8689cad to eabe360 Compare January 23, 2025 19:12
Copy link
Copy Markdown
Contributor

@jamesopstad jamesopstad left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few code comments and some changes to the readme.

Comment thread packages/vite-plugin-cloudflare/src/plugin-config.ts Outdated
Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
Comment thread packages/vite-plugin-cloudflare/playground/dev-vars/tsconfig.node.json Outdated
Comment thread packages/vite-plugin-cloudflare/playground/dev-vars/src/index.ts Outdated
Comment thread packages/vite-plugin-cloudflare/playground/dev-vars/.gitignore Outdated
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
Comment thread .changeset/orange-camels-hunt_wrangler.md Outdated
Comment thread .changeset/orange-camels-hunt_vite-plugin.md Outdated
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from 3bb0dd1 to 5cc21f3 Compare January 24, 2025 18:06
Comment thread .changeset/orange-camels-hunt_wrangler.md Outdated
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
Comment thread packages/vite-plugin-cloudflare/README.md Outdated
Comment on lines 2 to 3
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
MY_DEV_VAR_A = "my .dev.vars staging variable A"
MY_DEV_VAR_B = "my .dev.vars staging variable B"
MY_DEV_VAR_A = "my .dev.vars.staging variable A"
MY_DEV_VAR_B = "my .dev.vars.staging variable B"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I prefer not to have the . so that it is clearer that this this is a manually inputted string (otherwise without seeing the implementation someone could thing that this was generated by something like my ${devVarsFileName} variable A)

if you find it confusing I can change the strings to something like MY_DEV_VAR_A = "my .dev.vars variable A for the staging environment (or something like that)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note, mostly unrelated to this PR:

The vite-plugin catalog pins @types/node but that will not work as expected because of

		"overrides": {
			// ...      
			"@types/node": "$@types/node"
		},

So there would need to be a more specific override there for the plugin package

Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there any benefit in having a variable here (i.e. do we expect to add more files)
Otherwise it looks looks inlining would be less convoluted

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the benefit of the variable are:

  • it is nicer to have if we can indeed add more files in the future
  • if is a self documenting variable, the variable name helps (at least to me) understanding that it does

those are not huge benefits I don't mind inlining the array if that's what you strongly prefer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I admit I am not convinced this needs a variable. But I don't care strongly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

me neither 😅, as I said I prefer having the variable but it's not a huge deal either way 🙂

Comment thread packages/vite-plugin-cloudflare/src/plugin-config.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Looks good - with my limited understanding of the plugin.

But I always find things to comment ;)

@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from 23c0acd to b146539 Compare January 27, 2025 11:04
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I admit I am not convinced this needs a variable. But I don't care strongly.

Comment thread packages/vite-plugin-cloudflare/src/plugin-config.ts Outdated
Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
dario-piotrowicz and others added 12 commits January 28, 2025 00:37
the changes here add tests for making sure that the vite plugin correctly
reads secrets from `.dev.vars` files with and without a specified environment

as part of this wragler's `unstable_getMiniflareWorkerOptions` utility needed
to also be updated to accept the environment name as its second parameter
Update md files

Co-authored-by: James Opstad <13586373+jamesopstad@users.noreply.github.com>
Co-authored-by: James Opstad <13586373+jamesopstad@users.noreply.github.com>
Co-authored-by: Victor Berchet <victor@suumit.com>
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7850/dev-vars-in-vite-plugin branch from 744f80b to a39093d Compare January 28, 2025 00:37
@dario-piotrowicz dario-piotrowicz merged commit de6fa18 into main Jan 28, 2025
@dario-piotrowicz dario-piotrowicz deleted the dario/7850/dev-vars-in-vite-plugin branch January 28, 2025 14:52
@edmundhung edmundhung mentioned this pull request Jan 28, 2025
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run wrangler + vite-plugin e2e tests on a PR vite-plugin Relating to the `@cloudflare/vite-plugin` package

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Support .dev.vars in Vite plugin

4 participants