fix: eagerly initialise dynamic env vars from process.env at module-eval time - #1
Conversation
…-eval time Source PR: sveltejs#16303 Source head: 91afeee
⛔ Shipwright · BlockedRecommendation: do not merge PR #1 · Tier
Findings (10)
Fireworks usage: 37,049 input · 1,204 output · 38,253 total tokens · $0.0089 · 16s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| `Entrypoint file ${entrypoint} not found. This is probably a bug in your adapter.` | ||
| ); | ||
| } | ||
| if (!existsSync(initializer)) { |
There was a problem hiding this comment.
Shipwright · CRITICAL
The 'instrument' method now unconditionally checks 'existsSync(initializer)', but 'initializer' is an optional parameter.
Impact: The 'instrument' method now unconditionally checks 'existsSync(initializer)', but 'initializer' is an optional parameter. Any existing adapter that calls 'builder.instrument' without passing 'initializer' will now throw 'Instrumentation initializer undefined not found', breaking the API for all non-updated adapters. The diff does not add a default or guard for the missing parameter.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| `Entrypoint file ${entrypoint} not found. This is probably a bug in your adapter.` | ||
| ); | ||
| } | ||
| if (!existsSync(initializer)) { |
There was a problem hiding this comment.
Shipwright · HIGH
The 'instrument' method now unconditionally checks 'existsSync(initializer)', but 'initializer' is an optional parameter.
Impact: The 'instrument' method now unconditionally checks 'existsSync(initializer)', but 'initializer' is an optional parameter. Any existing adapter that calls 'builder.instrument' without passing 'initializer' will now throw 'Instrumentation initializer undefined not found', breaking the API for all non-updated adapters. The diff does not add a default or guard for the missing parameter.
Suggested fix: Fix the review finding before release.
| serverDirectory = `${config.outDir}/output/server` | ||
| }) { | ||
| const provider = path.join(outputDirectory, '__sveltekit_env.js'); | ||
| write(provider, environment ?? 'export default process.env;'); |
There was a problem hiding this comment.
Shipwright · HIGH
'createInstrumentationInitializer' writes the provider file using 'environment ??
Impact: 'createInstrumentationInitializer' writes the provider file using 'environment ?? 'export default process.env;''. If 'environment' is an empty string (a valid falsy value that could be intentionally supplied), it silently falls back to 'process.env' instead of using the provided empty environment, causing incorrect env population.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| const initializer = path.join(outputDirectory, '__sveltekit_env_init.js'); | ||
| write( | ||
| initializer, | ||
| create_env_module({ |
There was a problem hiding this comment.
Shipwright · HIGH
The generated initializer imports 'set_env' from '${serverDirectory}/env.js' and calls it with the environment object.
Impact: The generated initializer imports 'set_env' from '${serverDirectory}/env.js' and calls it with the environment object. If 'env.js' does not exist or does not export 'set_env', the generated module will fail at runtime. The method does not verify that 'serverDirectory/env.js' exists before generating the initializer, so adapters using a custom 'serverDirectory' without an 'env.js' will produce a broken build.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| const provider = path.join(outputDirectory, '__sveltekit_env.js'); | ||
| write(provider, environment ?? 'export default process.env;'); | ||
|
|
||
| const initializer = path.join(outputDirectory, '__sveltekit_env_init.js'); |
There was a problem hiding this comment.
Shipwright · HIGH
The generated initializer uses 'to_import_specifier' on the relative path from the initializer to the provider, but the provider path is computed relative to 'outputDirectory' whil
Impact: The generated initializer uses 'to_import_specifier' on the relative path from the initializer to the provider, but the provider path is computed relative to 'outputDirectory' while the initializer is also in 'outputDirectory'. If 'outputDirectory' is not the same as the directory containing the entrypoint, the relative path may be incorrect, causing the import to fail at runtime.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| serverDirectory = `${config.outDir}/output/server` | ||
| }) { | ||
| const provider = path.join(outputDirectory, '__sveltekit_env.js'); | ||
| write(provider, environment ?? 'export default process.env;'); |
There was a problem hiding this comment.
Shipwright · MEDIUM
'createInstrumentationInitializer' writes the provider file using 'environment ??
Impact: 'createInstrumentationInitializer' writes the provider file using 'environment ?? 'export default process.env;''. If 'environment' is an empty string (a valid falsy value that could be intentionally supplied), it silently falls back to 'process.env' instead of using the provided empty environment, causing incorrect env population.
Suggested fix: Fix the review finding before release.
| const initializer = path.join(outputDirectory, '__sveltekit_env_init.js'); | ||
| write( | ||
| initializer, | ||
| create_env_module({ |
There was a problem hiding this comment.
Shipwright · MEDIUM
The generated initializer imports 'set_env' from '${serverDirectory}/env.js' and calls it with the environment object.
Impact: The generated initializer imports 'set_env' from '${serverDirectory}/env.js' and calls it with the environment object. If 'env.js' does not exist or does not export 'set_env', the generated module will fail at runtime. The method does not verify that 'serverDirectory/env.js' exists before generating the initializer, so adapters using a custom 'serverDirectory' without an 'env.js' will produce a broken build.
Suggested fix: Fix the review finding before release.
| const provider = path.join(outputDirectory, '__sveltekit_env.js'); | ||
| write(provider, environment ?? 'export default process.env;'); | ||
|
|
||
| const initializer = path.join(outputDirectory, '__sveltekit_env_init.js'); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The generated initializer uses 'to_import_specifier' on the relative path from the initializer to the provider, but the provider path is computed relative to 'outputDirectory' whil
Impact: The generated initializer uses 'to_import_specifier' on the relative path from the initializer to the provider, but the provider path is computed relative to 'outputDirectory' while the initializer is also in 'outputDirectory'. If 'outputDirectory' is not the same as the directory containing the entrypoint, the relative path may be incorrect, causing the import to fail at runtime.
Suggested fix: Fix the review finding before release.
1 participant
closes sveltejs#14286
Dynamic
$app/envvalues are normally populated whenServer.init()callsset_env(). The problem is thatinstrumentation.server.jsdeliberately runs beforeServer.init(), so importing$app/envthere gives youundefined. More subtly, instrumentation can share a chunk with application code and cause that code to evaluate early too, permanently capturing those undefined values at module scope.Basically, instrumentation should run before the application, but not before the minimum runtime state it is allowed to observe exists.
This adds an environment initializer to
builder.instrument(). Adapters provide the final env module location and generate the small platform-specific initializer; Kit handles the relative import, imports it before instrumentation, and only then dynamically imports the application entrypoint. The same ordering now applies in dev and preview.The official adapters initialize from their native runtime source:
process.envenvfromcloudflare:workersDeno.env.toObject()Added tests for generated import paths, custom facades, module evaluation order, filename collisions, and a built adapter-node app reading dynamic env at instrumentation module scope.
Source merge-base:
e7c74bf8227dc228cb36e822eaff5992569b830aSource head:
91afeeed060aede8c09439926c818ec6f8d297f8