Skip to content

Allow unsafe param to pass through vite build#7909

Merged
penalosa merged 3 commits intomainfrom
byule/fix-vite-wrangler-unsafe
Jan 27, 2025
Merged

Allow unsafe param to pass through vite build#7909
penalosa merged 3 commits intomainfrom
byule/fix-vite-wrangler-unsafe

Conversation

@byule
Copy link
Copy Markdown
Contributor

@byule byule commented Jan 26, 2025

@jamesopstad this allows unsafe params to pass through the wrangler.json file while using vite as long as unsafe isn't set to an empty object. At least according to the comment, this was done for a reason to prevent empty objects from passing through, but seemed to do it unconditionally (even if not empty).

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: tested locally
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: don't apply to the vite plugin
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: small bugfix

@byule byule requested a review from a team January 26, 2025 15:43
@byule byule requested a review from a team as a code owner January 26, 2025 15:43
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 26, 2025

🦋 Changeset detected

Latest commit: ca9ef16

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

This PR includes changesets to release 1 package
Name Type
@cloudflare/vite-plugin 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 26, 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/12993643851/npm-package-wrangler-7909

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

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

Or you can use npx with this latest build directly:

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

cloudflare-workers-bindings-extension:

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

create-cloudflare:

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

@cloudflare/kv-asset-handler:

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

miniflare:

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

@cloudflare/pages-shared:

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

@cloudflare/unenv-preset:

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

@cloudflare/vite-plugin:

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

@cloudflare/vitest-pool-workers:

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

@cloudflare/workers-editor-shared:

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

@cloudflare/workers-shared:

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

@cloudflare/workflows-shared:

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

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.

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.

Thanks Ben! I'll make a note about fixing the root of the issue too so we can remove this.

@penalosa
Copy link
Copy Markdown
Contributor

@jamesopstad this could probably do with a test, but I'm not super familiar with the testing setup for the vite plugin—do you know where the right place to add one would be?

@byule byule force-pushed the byule/fix-vite-wrangler-unsafe branch from 480889c to cec5ea0 Compare January 27, 2025 14:18
@workers-devprod workers-devprod added the e2e Run wrangler + vite-plugin e2e tests on a PR label Jan 27, 2025
@jamesopstad
Copy link
Copy Markdown
Contributor

It turns out that @petebacondarwin merged a PR to allow the empty object for unsafe in Wrangler in December (#7461). Sorry I forgot about that. I still think we need to only pass it through if it's explicitly set by the user, however, or they will get the "unsafe" fields are experimental and may change or break at any time. error when they deploy. Happy to merge this as is for now and then we can have another look at the behaviour in Wrangler (perhaps to set the default unsafe value to undefined rather than an empty object).

@jamesopstad
Copy link
Copy Markdown
Contributor

@jamesopstad this could probably do with a test, but I'm not super familiar with the testing setup for the vite plugin—do you know where the right place to add one would be?

Most tests are written as integration tests in the playground directory. Probably not necessary for this though as the fix is just to avoid a warning.

Comment thread packages/vite-plugin-cloudflare/src/index.ts Outdated
@penalosa penalosa force-pushed the byule/fix-vite-wrangler-unsafe branch from cec5ea0 to f776a2a Compare January 27, 2025 16:10
@penalosa penalosa removed the e2e Run wrangler + vite-plugin e2e tests on a PR label Jan 27, 2025
Co-authored-by: James Opstad <13586373+jamesopstad@users.noreply.github.com>
@penalosa penalosa merged commit 0b79cec into main Jan 27, 2025
@penalosa penalosa deleted the byule/fix-vite-wrangler-unsafe branch January 27, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants