test(adapter-node): boot the built output when instrumentation.server.js is present - #16305
Closed
SobolievOleksii wants to merge 9 commits into
Closed
Conversation
….js is present Adds a fixture app + boot test that builds with adapter-node and starts build/index.js, asserting that (1) the server boots at all and (2) a module-scope $app/env/private read observes the runtime value. Currently red on version-3: with an instrumentation file present, the adapter's env.js is emitted outside the ENV_PREFIX replace filter and the server crashes at startup with 'ReferenceError: ENV_PREFIX is not defined'. Also covers the startup-crash and env-ordering regressions discussed in sveltejs#16288 / sveltejs#16302 / sveltejs#16303. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/c5b321362dc3b274090bd99f25b99f1c4b79601fOpen in Note This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed. |
|
6 tasks
Rich-Harris
pushed a commit
that referenced
this pull request
Jul 20, 2026
) Fixes #13718. Supersedes #13719, credit to @HoldYourWaffle for the diagnosis and approach. The trailing slash redirects use absolute pathnames, so any prefix a parent server strips before the request reaches kit (an Express mount, #13702) is dropped and the browser lands outside the app. The prefix never reaches Polka's parser, so it can't be reconstructed server side. A relative Location resolves against the full browser URL and is prefix-agnostic. Per the review on #13719, the helper is implemented in kit rather than adding `get-relative-path`, stays internal instead of being exported from `@sveltejs/kit/node`, and adapter-node gets a local copy since its handler is bundled standalone. Both call sites only produce pathnames differing by a trailing slash, so it handles exactly that case. The #2515 guard becomes unnecessary, a relative reference never starts with `/`. The options app trailingSlash tests now assert the Location values and fail on the old code. The prerender crawler resolves locations with `new URL()`, so it follows the relative form unchanged. Verified end to end with an adapter-node build mounted under a stripped prefix, both the SSR and the prerendered redirect. No adapter-node test app here since #16305 is establishing that infrastructure. --- ### Please don't delete this checklist! Before submitting the PR, please make sure you do the following: - [x] 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 - [x] This message body should clearly illustrate what problems it solves. - [x] Ideally, include a test that fails without this PR but passes with it. ### Tests - [x] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check` ### Changesets - [x] 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:`. ### Edits - [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed. --------- Co-authored-by: HoldYourWaffle <holdyourwaffle@gmail.com>
elliott-with-the-longest-name-on-github
added
the
needs-platform-tests
…trumentation-boot-test # Conflicts: # packages/adapter-node/package.json
- pnpm-lock.yaml: re-add the fixture importer against the current catalog (vite 8.2.1 / svelte 5.56.8 / vite-plugin-svelte 7.3.0), matching the sibling basic app so no extra package versions enter the lockfile - drop the fixture's prepare script, gone from adapter-node test apps upstream (sveltejs#16729) - tsconfig extends $app/tsconfig instead of ./.svelte-kit/tsconfig.json Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
Thank you for the PR but I think we'll close it in favour of #16303 which includes a test without introducing a new test app |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
A boot test for adapter-node with a server instrumentation file present — the coverage gap behind #16288 and the review findings on #16302/#16303. The fixture app (under
packages/adapter-node/test/apps/instrumentation, next to the existingbasicapp) builds with the real adapter and thentest/boot.jsstartsbuild/index.jsand asserts:$app/env/privateread observes the runtime value (the "capturedundefined" class from instrumentation.server.js can evaluate app modules before Server.init() when the bundler colocates a shared dependency ($env/dynamic/private silently undefined) #16288).Wired into
pnpm testof the adapter (test:unit+test:apps), so the existingtest:othersCI job picks it up. The existingbasicapp covers hooks/streaming behaviour but nothing boots the output withsrc/instrumentation.server.jspresent — which is exactly the path #16303 changes.Results
version-3, built on ubuntu (this PR's CI)captured === liveversion-3, built on Windowscaptured === live(see history note below)SyntaxError: The requested module './env.js' does not provide an export named 'set_env'— the generated__sveltekit_env_init.jsfacade resolves the adapter's ownenv.jsinstead of kit's env module. This is a name collision in the emitted output, so it is platform-independent — this test would catch it in CI once the branch includes itHistory note (updated 2026-08-28): when this PR was opened, the Windows build failed with
ReferenceError: ENV_PREFIX is not defined— the adapter's constant-replace filter received non-posixified Windows paths, soenv.jsescaped the replacement. That turned out to be #16365, fixed by #16367 (merged 2026-07-16). After rebasing this branch onto currentversion-3, the boot test passes on Windows too — and had a test like this run on a Windows leg earlier, it would have caught #16365 before a user did.So on CI this PR is green, guards the instrumentation boot path going forward, and would have flagged #16303.
Refs #16288.