Skip to content

breaking: Move cloudflare bindings from platform to cloudflare:workers - #2

Open
anurag6569201 wants to merge 1 commit into
qa/agent-sveltejs-kit/pr-02-16754/basefrom
qa/agent-sveltejs-kit/pr-02-16754/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-sveltejs-kit/pr-02-16754/basefrom
qa/agent-sveltejs-kit/pr-02-16754/head

Conversation

@anurag6569201

Copy link
Copy Markdown

Breaking sveltejs#16705 into smaller pieces; this is the first step of making the dev environment closer to the workerd runtime. Instead of accessing Cloudflare bindings on platform.env, you would do the following:

import { env } from 'cloudflare:workers';

export function GET({ request }) {
  const value = await env.KV.get('key'); // instead of platform.env
  const userCountry = request.cf.country; // instead of platform.cf
  const cache = caches.open('name'); // instead of platform.caches
}

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Source merge-base: 3c3a27bf67a26164266e132dbb7f9266643cf724
Source head: 190b18918d5b1c625fa0d8d668fa2578a190c0e8

@shipwright-agent

Copy link
Copy Markdown

⛔ Shipwright · Blocked

Recommendation: do not merge PR #2 · Tier T3
Checks: 0 total · 0 needing attention

Next step: resolve the blocking findings before merge.

Findings (22)

  • CRITICAL The test for Request.cf uses 'request.get('cf')' without a leading slash, so it will not hit the '/cf' route and will fail or assert against a 404 response. · packages/adapter-cloudflare/test/apps/workers/test/test.js:15
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL 'replace_stub' recursively reads every file in the server directory as UTF-8 and rewrites it if the stub string appears. · packages/adapter-cloudflare/index.js:333
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL 'replace_stub' is called on 'builder.getServerDirectory()' before that directory is guaranteed to exist. · packages/adapter-cloudflare/index.js:88
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL The virtual module resolver returns '{ id: stub_import, external: true }' for 'cloudflare:workers', but the stub file may not be resolvable in the published package or at runtime, · packages/adapter-cloudflare/index.js:250
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The test for Request.cf uses 'request.get('cf')' without a leading slash, which will not resolve to the '/cf' route and will likely return a 404 or unexpected response, causing the · packages/adapter-cloudflare/test/apps/workers/test/test.js:15
    • Fix: Fix the review finding before release.
  • HIGH The 'replace_stub' function recursively reads and writes every file in the server directory, but it does not restrict itself to JavaScript/TypeScript files. · packages/adapter-cloudflare/index.js:333
    • Fix: Fix the review finding before release.
  • HIGH The 'virtual_workers_module' plugin's 'resolveId' handler returns '{ id: stub_import, external: true }' for the 'cloudflare:workers' specifier. · packages/adapter-cloudflare/index.js:250
    • Fix: Fix the review finding before release.
  • HIGH The 'replace_stub' function is called with 'builder.getServerDirectory()' before the server directory is guaranteed to exist. · packages/adapter-cloudflare/index.js:88
    • Fix: Fix the review finding before release.
  • …and 14 more findings in the check details.

Fireworks usage: 32,375 input · 2,489 output · 34,864 total tokens · $0.0088 · 26s · 0 fix iteration(s)

Open the Shipwright check for full evidence and the audit bundle. Use /shipwright rerun to verify again.

expect(await res.text()).toBe('from wrangler.jsonc');
});

test('Request.cf', async ({ request }) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · CRITICAL

The test for Request.cf uses 'request.get('cf')' without a leading slash, so it will not hit the '/cf' route and will fail or assert against a 404 response.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

*/
function replace_stub(directory, stub_import) {
// recurse, find stub_import, replace with "cloudflare:workers"
const files = fs.readdirSync(directory);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · CRITICAL

'replace_stub' recursively reads every file in the server directory as UTF-8 and rewrites it if the stub string appears.

Impact: 'replace_stub' recursively reads every file in the server directory as UTF-8 and rewrites it if the stub string appears. Binary assets such as wasm, images, or fonts can be corrupted or cause the build to throw, breaking the generated worker output.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

expect(await res.text()).toBe('from wrangler.jsonc');
});

test('Request.cf', async ({ request }) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The test for Request.cf uses 'request.get('cf')' without a leading slash, which will not resolve to the '/cf' route and will likely return a 404 or unexpected response, causing the

Impact: The test for Request.cf uses 'request.get('cf')' without a leading slash, which will not resolve to the '/cf' route and will likely return a 404 or unexpected response, causing the test to fail.

Suggested fix: Fix the review finding before release.

*/
function replace_stub(directory, stub_import) {
// recurse, find stub_import, replace with "cloudflare:workers"
const files = fs.readdirSync(directory);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The 'replace_stub' function recursively reads and writes every file in the server directory, but it does not restrict itself to JavaScript/TypeScript files.

Impact: The 'replace_stub' function recursively reads and writes every file in the server directory, but it does not restrict itself to JavaScript/TypeScript files. It will read binary files (e.g., images, fonts, wasm) as UTF-8 and, if the stub string happens to appear in the binary data, corrupt them. More importantly, it rewrites files even when no replacement is needed only if the stub is present, but reading binary file…

Suggested fix: Fix the review finding before release.

*/
function replace_stub(directory, stub_import) {
// recurse, find stub_import, replace with "cloudflare:workers"
const files = fs.readdirSync(directory);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The 'replace_stub' function uses 'fs.readFileSync(file_path, 'utf8')' and 'fs.writeFileSync(file_path, contents.replaceAll(stub_import, 'cloudflare:workers'))' without checking if

Impact: The 'replace_stub' function uses 'fs.readFileSync(file_path, 'utf8')' and 'fs.writeFileSync(file_path, contents.replaceAll(stub_import, 'cloudflare:workers'))' without checking if the file is a text file. This can corrupt binary files in the server directory, such as '.wasm' files or images, if they contain the stub string. Even if they don't contain the stub, reading them as UTF-8 can throw an error or produce inva…

Suggested fix: Fix the review finding before release.


const value = await env.KV.get('key');
```
- `cf` is now a property of the `Request` object:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The migration example declares 'interface Request { cf: ...

Impact: The migration example declares 'interface Request { cf: ... }' but uses 'request.cf' on the SvelteKit request object without showing how 'cf' is actually exposed, making the documented API misleading.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

).cf = globalThis.__sveltekit_cloudflare_platform?.cf;
return request;
},
plugins: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

'getRequest' assigns 'request.cf = globalThis.__sveltekit_cloudflare_platform?.cf'; when the platform proxy is not initialized, this sets 'cf' to 'undefined', which can break downs

Impact: 'getRequest' assigns 'request.cf = globalThis.__sveltekit_cloudflare_platform?.cf'; when the platform proxy is not initialized, this sets 'cf' to 'undefined', which can break downstream code expecting an object.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

filter: { id: exactRegex('cloudflare:workers') },
handler() {
return {
id: stub_import,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The 'setup' function does not handle 'getPlatformProxy' rejection.

Impact: The 'setup' function does not handle 'getPlatformProxy' rejection. An invalid wrangler config or initialization failure will crash the dev/preview server without a clear error.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

*/
function replace_stub(directory, stub_import) {
// recurse, find stub_import, replace with "cloudflare:workers"
const files = fs.readdirSync(directory);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

'replace_stub' follows symlinks via 'fs.statSync' and recurses into linked directories, which can modify files outside the build output or loop indefinitely on circular symlinks.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


const value = await env.KV.get('key');
```
- `cf` is now a property of the `Request` object:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · MEDIUM

The migration guide example for 'cf' declares 'interface Request { cf: ...

Impact: The migration guide example for 'cf' declares 'interface Request { cf: ... }' in an ambient.d.ts file, but the code snippet uses 'request.cf' where 'request' is the SvelteKit request object, not the native 'Request'. The example does not demonstrate how 'cf' is actually exposed on the request object, making the documented API misleading.

Suggested fix: Fix the review finding before release.

append_headers
} from './utils.js';
import { exactRegex } from '@rolldown/pluginutils';
import { getRequest } from '@sveltejs/kit/node';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · MEDIUM

The 'replace_stub' function uses 'contents.replaceAll(stub_import, 'cloudflare:workers')' without escaping the stub_import string.

Impact: The 'replace_stub' function uses 'contents.replaceAll(stub_import, 'cloudflare:workers')' without escaping the stub_import string. Since 'stub_import' contains a random UUID and a query string, it is not a regex, so 'replaceAll' with a string is safe. However, the stub_import is generated with 'crypto.randomUUID()' and appended to the resolved path. If the build is run in an environment where 'crypto.randomUUID' is…

Suggested fix: Fix the review finding before release.

).cf = globalThis.__sveltekit_cloudflare_platform?.cf;
return request;
},
plugins: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · MEDIUM

The 'getRequest' function in the 'vite' object mutates the request object by assigning 'request.cf = globalThis.__sveltekit_cloudflare_platform?.cf'.

Impact: The 'getRequest' function in the 'vite' object mutates the request object by assigning 'request.cf = globalThis.__sveltekit_cloudflare_platform?.cf'. If 'globalThis.__sveltekit_cloudflare_platform' is undefined (e.g., when the platform proxy has not been set up yet), this assigns 'undefined' to 'request.cf', which may cause downstream code to fail when it expects 'cf' to be an object. The optional chaining only prev…

Suggested fix: Fix the review finding before release.

filter: { id: exactRegex('cloudflare:workers') },
handler() {
return {
id: stub_import,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · MEDIUM

The 'setup' function in 'virtual_workers_module' sets 'globalThis.__sveltekit_cloudflare_platform = proxy' and 'globalThis.caches = proxy.caches' but does not handle the case where

Impact: The 'setup' function in 'virtual_workers_module' sets 'globalThis.__sveltekit_cloudflare_platform = proxy' and 'globalThis.caches = proxy.caches' but does not handle the case where 'getPlatformProxy' throws. If the platform proxy fails to initialize (e.g., invalid config), the error is unhandled and will crash the dev server or preview server without a clear error message.

Suggested fix: Fix the review finding before release.

*/
function replace_stub(directory, stub_import) {
// recurse, find stub_import, replace with "cloudflare:workers"
const files = fs.readdirSync(directory);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · MEDIUM

The 'replace_stub' function does not handle symlinks.

Impact: The 'replace_stub' function does not handle symlinks. If the server directory contains symlinks to directories, 'fs.statSync' will follow the symlink and recurse into the target directory, potentially modifying files outside the build output. This can cause unintended file modifications or infinite recursion if there are circular symlinks.

Suggested fix: Fix the review finding before release.

append_headers
} from './utils.js';
import { exactRegex } from '@rolldown/pluginutils';
import { getRequest } from '@sveltejs/kit/node';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · LOW

'crypto.randomUUID()' is called at module load time; environments without secure-context 'crypto.randomUUID' support will throw before the adapter can run.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

/** @type {import('@cloudflare/workers-types').Request} */ (
/** @type {unknown} */ (request)
).cf = globalThis.__sveltekit_cloudflare_platform?.cf;
return request;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · LOW

The 'getRequest' function in the 'vite' object is defined as a method that takes 'options' and returns a request.

Impact: The 'getRequest' function in the 'vite' object is defined as a method that takes 'options' and returns a request. However, the 'vite' object is part of the adapter's return value, and the 'getRequest' function is not documented or typed as part of the adapter API. This may cause confusion or breakage if the adapter is used in a context where the 'vite' object is not expected to have a 'getRequest' method.

Suggested fix: Fix the review finding before release.

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.

1 participant