breaking: Move cloudflare bindings from platform to cloudflare:workers - #2
anurag6569201 wants to merge 1 commit into
Conversation
…kers` Source PR: sveltejs#16754 Source head: 190b189
⛔ Shipwright · BlockedRecommendation: do not merge PR #2 · Tier
Findings (22)
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 |
| expect(await res.text()).toBe('from wrangler.jsonc'); | ||
| }); | ||
|
|
||
| test('Request.cf', async ({ request }) => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 }) => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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: { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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: { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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:Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Source merge-base:
3c3a27bf67a26164266e132dbb7f9266643cf724Source head:
190b18918d5b1c625fa0d8d668fa2578a190c0e8