fix: ensure importing $app/navigation in unit tests work#14195
fix: ensure importing $app/navigation in unit tests work#14195Rich-Harris merged 10 commits intomainfrom
$app/navigation in unit tests work#14195Conversation
🦋 Changeset detectedLatest commit: f1c2d9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
||
| /** @type {Record<string, any>} */ | ||
| export const remote_responses = __SVELTEKIT_PAYLOAD__.data ?? {}; | ||
| // avoid referencing `__SVELTEKIT_PAYLOAD__` because it will be undefined unless this module was loaded by Vite |
There was a problem hiding this comment.
I don't fully understand yet why Vitest wouldn't load this through Vite module loader yet. If anything, this comment needs to be enhanced a bit to clarify this.
I also don't fully understand why this global is causing problems but the other globals in this file do not - would they also cause problems if we started using them for more than booleans, i.e. property access?
There was a problem hiding this comment.
You're right. I was completely misunderstanding the issue. Vite does perform the global variable replacements. It's just that our globalThis.__sveltekit_dev object is undefined unless it's defined by our server rendered page.
So the only issue was accessing a property on an undefined object. I'll just add an optional chain.
There was a problem hiding this comment.
Inside vite/index.js we have
// @ts-ignore this prevents a reference error if `client.js` is imported on the server
globalThis.__sveltekit_dev = {};I wonder why that isn't working? Is it because for Vitest the if (build) branch is used? If so, should we instead add a "is this Vitest starting this build if so add the globalThis in here like we do in dev"? Or could this cause problems in other places?
There was a problem hiding this comment.
Inside
vite/index.jswe have// @ts-ignore this prevents a reference error if `client.js` is imported on the server globalThis.__sveltekit_dev = {};I wonder why that isn't working? Is it because for Vitest the
if (build)branch is used?
Definitely not because of the if (build) branch. I can reproduce the issue here where the globalThis is not the same when logging it in the test file after running pnpm test: https://stackblitz.com/edit/vitejs-vite-tycr5kae
Maybe it's because test files are run in a different thread so they don't share the same globalThis? https://vitest.dev/guide/features.html#threads
…14195) * add fix and test * changeset * format * can't colocate vitest and playwright tests * Apply suggestion from @eltigerchino * Apply suggestion from @eltigerchino * Apply suggestion from @eltigerchino * clean up comment
3 participants
fixes #14143
This PR adds a condition before referencing the value of
__SVELTEKIT_PAYLOAD__so that it doesn't error when it doesn't exist during unit testing.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:.Edits