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

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dario-piotrowicz
Copy link
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)

Copy link

changeset-bot bot commented Jan 22, 2025

🦋 Changeset detected

Latest commit: 23c0acd

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

Copy link
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/12936365824/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/12936365824/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/12936365824/npm-package-cloudflare-workers-bindings-extension-7864 -O ./cloudflare-workers-bindings-extension.0.0.0-v9059e4169.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v9059e4169.vsix

create-cloudflare:

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

@cloudflare/kv-asset-handler:

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

miniflare:

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

@cloudflare/pages-shared:

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

@cloudflare/unenv-preset:

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

@cloudflare/vite-plugin:

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

@cloudflare/vitest-pool-workers:

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

@cloudflare/workers-editor-shared:

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

@cloudflare/workers-shared:

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

@cloudflare/workflows-shared:

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

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


wrangler@3.105.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241230.2
workerd 1.20241230.0 1.20241230.0
workerd --version 1.20241230.0 2024-12-30

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 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
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review January 22, 2025 17:24
@dario-piotrowicz dario-piotrowicz requested review from a team as code owners January 22, 2025 17:24
@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 on lines +315 to +324
* Note: This resolves the .dev.vars file path following the same logic
* as `loadDotEnv` in `/packages/wrangler/src/config/index.ts`
* the two need to be kept in sync
Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Jan 23, 2025

Choose a reason for hiding this comment

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

this is not ideal.... ideally we should share the functionality 😓

we could do that by exposing some API from wrangler but that would feel a bit gnarly to me (since this seems like internal logic more than anything else to me)

in my opinion, one of the biggest strengths in having a monorepo setup would be to share code... so I would really consider having some internal package for shared logic such as this, in that way code could be shared very easily without needing to expose it publicly in any of our packages

anyways that's quite overkill for something as simple as this, but anyways I wanted to mention it just because I think setting up something like that can in general be very useful as this need to share code does come up from time to time (from my point of view, we pay the price of setting the thing up just once but then it pays off every time there's such a need).

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

packages/vite-plugin-cloudflare/src/plugin-config.ts Outdated Show resolved Hide resolved
packages/vite-plugin-cloudflare/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 203 to 206
copyDotDevDotVarsFileToOutputDir(
workerConfig.configPath,
resolvedPluginConfig.cloudflareEnv,
this.emitFile
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really picky so feel free to ignore but my preference would be to call this.emitFile here and for the function to just return the content rather than also writing the file. Makes it easier to see what's happening.

Suggested change
copyDotDevDotVarsFileToOutputDir(
workerConfig.configPath,
resolvedPluginConfig.cloudflareEnv,
this.emitFile
);
this.emitFile({
type: "asset",
fileName: ".dev.vars",
source: getDotDevDotVarsContent(
workerConfig.configPath,
resolvedPluginConfig.cloudflareEnv
)
});

Copy link
Member Author

Choose a reason for hiding this comment

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

With your suggestion is sounds like you'd want to always save the .dev.vars file even if there is no content to save? That'd be ok, but I think it'd be nicer and clearer if we saved the file only when actually needed (keep in mind that this.emitFile also logs the information to the user)

So I've applied a similar change: 3bb0dd1

Let me know what you think 🙂

(I've also added a comment to clarify why we're saving such a file, although I understand that it is a bit unnecessary)

packages/vite-plugin-cloudflare/README.md Outdated Show resolved Hide resolved
packages/vite-plugin-cloudflare/README.md Outdated Show resolved Hide resolved
.changeset/orange-camels-hunt_wrangler.md Outdated Show resolved Hide resolved
.changeset/orange-camels-hunt_vite-plugin.md Outdated Show resolved Hide resolved
dario-piotrowicz and others added 7 commits January 24, 2025 18:06
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>
@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 on lines +2 to +3
MY_DEV_VAR_A = "my .dev.vars staging variable A"
MY_DEV_VAR_B = "my .dev.vars staging variable B"
Copy link
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
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)

"@cloudflare/vite-plugin": "workspace:*",
"@cloudflare/workers-tsconfig": "workspace:*",
"@cloudflare/workers-types": "^4.20241230.0",
"typescript": "catalog:vite-plugin",
Copy link
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

this.emitFile({
type: "asset",
fileName: ".assetsignore",
source: "wrangler.json",
source: `${filesToAssetsIgnore.join("\n")}\n`,
Copy link
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
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

Comment on lines +86 to +90
const { CLOUDFLARE_ENV: cloudflareEnv } = vite.loadEnv(
viteEnv.mode,
root,
""
);
Copy link
Contributor

Choose a reason for hiding this comment

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

rant:

Seeing this annoys me, same thing as with function(path, config, true, true, false, "")

	const { CLOUDFLARE_ENV: cloudflareEnv } = vite.loadEnv(
		viteEnv.mode,
		root,
		/* what is this??? usually the name of the parameter */""
	);

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I should start reviewing PR in VSCode instead....

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, this could be cleared if we owned the API and used a config object instead

with Vite's API we can still make this clearer by having a temporary documenting variable, but that might not be what you personally want since before you said that you find such a variable "convoluted"

Copy link
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 ;)

Co-authored-by: Victor Berchet <victor@suumit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR vite-plugin Relating to the `@cloudflare/vite-plugin` package
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

Support .dev.vars in Vite plugin
3 participants