Skip to content

SvelteSet does not behave correctly in Vitest #13961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abdel-17 opened this issue Oct 27, 2024 · 33 comments · Fixed by #14026 or #14096
Closed

SvelteSet does not behave correctly in Vitest #13961

abdel-17 opened this issue Oct 27, 2024 · 33 comments · Fixed by #14026 or #14096

Comments

@abdel-17
Copy link

Describe the bug

The following test fails.

import { SvelteSet } from "svelte/reactivity";
import { expect, test } from "vitest";

class Group {
	selected = new SvelteSet<string>();
}

class Item {
	group: Group;
	id: string;

	constructor(group: Group, id: string) {
		this.group = group;
		this.id = id;
	}

	#selected = $derived.by(() => this.group.selected.has(this.id));

	get selected() {
		return this.#selected;
	}

	set selected(value: boolean) {
		if (value) {
			this.group.selected.add(this.id);
		} else {
			this.group.selected.delete(this.id);
		}
	}
}

test("Item.selected", () => {
	const cleanup = $effect.root(() => {
		const group = new Group();
		const item = new Item(group, "foo");

		expect(group.selected.has("foo")).toBe(false);
		expect(item.selected).toBe(false);

		item.selected = true;
		expect(group.selected.has("foo")).toBe(true);	// this is fine
		expect(item.selected).toBe(true);		// but this throws an error

		item.selected = false;
		expect(group.selected.has("foo")).toBe(false);
		expect(item.selected).toBe(false);
	});

	cleanup();
});

This issue only happens in Vitest. If you run the reproduction provided below, you will find that the same code works fine in the browser. Both values are the same, as expected.

Reproduction

https://github.com/abdel-17/svelte-vitest-set-bug-repro

Logs

No response

System Info

System:
    OS: macOS 15.0.1
    CPU: (8) x64 Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
    Memory: 3.80 GB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.9.0 - /usr/local/bin/node
    Yarn: 1.22.22 - /usr/local/bin/yarn
    npm: 10.8.3 - /usr/local/bin/npm
    pnpm: 9.12.0 - /usr/local/bin/pnpm
    bun: 1.1.31 - /usr/local/bin/bun
  Browsers:
    Safari: 18.0.1

Severity

annoyance

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

Do you have JSDOM set as your Vitest env?

@abdel-17
Copy link
Author

Yes, it's enabled in the reproduction repo. Also, side note, it's not very clear from the docs that jsdom is needed for effects to work.

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

Svelte 5 uses microtasks for scheduling, you can force things to be sync by using flushSync() after mutations:

item.selected = true;
flushSync();

However, given you're not using effects, I doubt that will change things here. You can try wrapping your logic in an effect (within the $effect.root, such as $effect.pre too).

@abdel-17
Copy link
Author

I did try flushSync but it didn't work. I'm a bit confused though. The code is already wrapped in an effect root.

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

Yeah it likely won't make a difference. I'll take a look into this on Monday :) I'm away without my laptop this weekend

@trueadm trueadm self-assigned this Oct 27, 2024
@brunnerh
Copy link
Member

I copied that code into my local SvelteKit project and it just worked.
The project has barely any config changes regarding tests:

export default defineConfig((cfg) => ({
  // ...
  test: {
    // (added before a change was made that allows `.svelte.` in other positions)
    include: ['src/**/*.test.svelte.{js,ts}'],
  }
}));

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

@brunnerh What if you do this without SvelteKit? i.e default sv?

@brunnerh
Copy link
Member

sv does not have a non-SvelteKit option right now as far as I know (sveltejs/cli#168)

@brunnerh
Copy link
Member

Did npm create vite with the Svelte option and it works there as well.

@abdel-17
Copy link
Author

Did npm create vite with the Svelte option and it works there as well.

Can you clone the reproduction repo and test if it doesn't work for you? That seems very strange...

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

I see an issue though:

https://github.com/abdel-17/svelte-vitest-set-bug-repro/blob/main/src/demo.svelte.spec.ts

This path won't work AFAIK. The vite-plugin-svelte module only handles *.svelte.ts and *.svelte.js files.

Maybe try:

https://github.com/abdel-17/svelte-vitest-set-bug-repro/blob/main/src/demo.spec.svelte.ts

@brunnerh
Copy link
Member

brunnerh commented Oct 27, 2024

No, that was changed (as noted in my comment in my config).
And it would error way earlier than it does due to the runes not being defined.


Tested the reproduction and it fails for whatever reason.

- Expected
+ Received

- true
+ false

 ❯ src/demo.svelte.spec.ts:42:25
     40|   item.selected = true;
     41|   expect(group.selected.has("foo")).toBe(true);
     42|   expect(item.selected).toBe(true);
       |                         ^
     43|
     44|   item.selected = false;
 ❯ update_reaction node_modules/.pnpm/[email protected]/node_modules/svelte/src/internal/client/runtime.js:327:56
 ❯ Module.update_effect node_modules/.pnpm/[email protected]/node_modules/svelte/src/internal/client/runtime.js:455:18
 ❯ create_effect node_modules/.pnpm/[email protected]/node_modules/svelte/src/internal/client/reactivity/effects.js:98:26
 ❯ Module.effect_root node_modules/.pnpm/[email protected]/node_modules/svelte/src/internal/client/reactivity/effects.js:222:17
 ❯ src/demo.svelte.spec.ts:33:8

@abdel-17
Copy link
Author

I see an issue though:

https://github.com/abdel-17/svelte-vitest-set-bug-repro/blob/main/src/demo.svelte.spec.ts

This path won't work AFAIK. The vite-plugin-svelte module only handles *.svelte.ts and *.svelte.js files.

Maybe try:

https://github.com/abdel-17/svelte-vitest-set-bug-repro/blob/main/src/demo.spec.svelte.ts

Screenshot 2024-10-28 at 2 49 03 AM

Also tried it and it doesn't work.

@trueadm
Copy link
Contributor

trueadm commented Oct 27, 2024

None of this makes sense, unless vite is doing some module isolation logic that means that the runtime and the tests aren't connected. Like I said, I have no computer till Monday :/

@benmccann
Copy link
Member

I think the issue is that vitest is running the server runtime (vitest-dev/vitest#2603). If you set resolve: { conditions: ['browser'] } then the test passes.

@benmccann
Copy link
Member

benmccann commented Oct 28, 2024

I really don't know what the right solution is here. I guess that Svelte needs browser to get the reactive version. However, vitest specifies node because it's running on Node.js. In general, the browser condition is going to cause problems because you'll end up with code that uses window, etc. And in fact, I think vite won't even let you specify both browser and node at the same time.

It's also possible that you would have some luck with browser mode: https://vitest.dev/guide/browser/

@benmccann
Copy link
Member

Hmm. What wasn't this an issue with Svelte 4 though?

@dummdidumm
Copy link
Member

This sums it up well: testing-library/svelte-testing-library#222 (comment)

We should add something along those lines to our documentation. The aliasing is a bit hacky, ideally we'd expose it in a robust way, but I'm not sure if there's a good way to do that via vite-plugin-svelte for example (maybe @dominikg has a good idea how to go about it).

@dominikg
Copy link
Member

svelte testing library published a utility plugin for this, but if it is needed for unit testing runic code in .svelte.ts files too we should think about adding it to vps. needs options to disable it though bc there can be cases where you don't want the browser condition to apply (testing ssr, dependencies that use browser globals with it)

https://testing-library.com/docs/svelte-testing-library/setup#vitest

@paoloricciuti
Copy link
Member

I need to open an issue on testing library about this but we should also include it in the docs: if you set resolve codition browser env-esm will rightrully return browser true...but this means that all the tests that don't need JSDom will fail because svelte is trying to access requestAnimationFrame when browser is true which is not defined. I solved this my mocking the function to a noop but that might not be enough if you also need to test transitions and animations (in the sense that you might need a more nuanced mock or just mock them on the specific pages where you don't need JSDom.

All of this imho is worth adding to the docs, it might be a bit complicated but i spent 4 hrs last day trying to figure out what was happening.

@dummdidumm
Copy link
Member

The issue I referenced goes into detail about this, and proposes using alias as a solution (which in fact we're doing too in our test suite). @dominikg is this something we could somehow add to v-p-s, too, in a robust way?

Example:

User does

import { svelte } from '@sveltejs/vite-plugin-svelte';
// ...

export default defineConfig({
  plugins: [svelte({ testing: { browserAlias: true } })] // name open to bikeshedding
});

That means that v-p-s does something like this

const pkg = JSON.parse(fs.readFileSync('svelte/package.json', 'utf8'));

export default defineConfig({
	resolve: {
		alias: [
			{
				find: /^svelte\/?/,
				customResolver: (id, importer) => {
					// For some reason this turns up as "undefined" instead of "svelte/"
					const exported = pkg.exports[id === 'undefined' ? '.' : id.replace('undefined', './')];
					if (!exported) return;

					// When running the server version of the Svelte files,
					// we also want to use the server export of the Svelte package
					return path.resolve(
						'packages/svelte',
						/* some conditional if people don't want this for certain files */
							? exported.default
							: exported.browser ?? exported.default
					);
				}
			}
		]
	}
}); 

@dominikg
Copy link
Member

according to https://github.com/rollup/plugins/tree/master/packages/alias#custom-resolvers customResolver is a customized instance of rollup plugin node-resolve, not a resolve function.
Also not sure what packages/svelte refers to here as our published package does not contain that.

The alias solution seems more hackish than adding the browser condition where needed.
Is svelte really eagerly accessing rAF when browser is set or is that just for transitions/animations? and jsdom does not supply it? wtf

@paoloricciuti
Copy link
Member

Is svelte really eagerly accessing rAF when browser is set or is that just for transitions/animations? and jsdom does not supply it? wtf

Eagerly in this module

const request_animation_frame = BROWSER ? requestAnimationFrame : noop;

which is re-exported in /client/internal

export { raf } from './timing.js';

But JSDom actually supply it...the problem is when you have a file that doesn't need JSDom (because it's just testing some utility for example but that utility is exported from a file where svelte is also imported from some unrelated stuff)

@dominikg
Copy link
Member

#12133 mentions you can override raf.tick.

why do we rely on BROWSER instead of requestAnimationFrame ?? noop though ?

@brunnerh
Copy link
Member

That would throw a reference error, you would at least need:

globalThis.requestAnimationFrame ?? noop
// or
typeof requestAnimationFrame == 'undefined' ? noop : requestAnimationFrame

@paoloricciuti
Copy link
Member

#12133 mentions you can override raf.tick.

why do we rely on BROWSER instead of requestAnimationFrame ?? noop though ?

Yeah but the problem here is just accessing requestAnimationFrame which, as @brunnerh suggested, will throw on non JSDom environments.

@trueadm trueadm removed their assignment Oct 28, 2024
@benmccann
Copy link
Member

Does jsdom mock browser globals like window and document?

why do we rely on BROWSER instead of requestAnimationFrame ?? noop though ?

One reason is for smaller bundle size. You're making the browser runtime larger if you have to add requestAnimationFrame ?? noop instead of just being able to assume it.

@paoloricciuti
Copy link
Member

Does jsdom mock browser globals like window and document?

I think it does but again the problem is for the files where you don't have to use JSDom because is just js.

@benmccann
Copy link
Member

benmccann commented Oct 28, 2024

Really what we want is a way to say that we're running browser code in Node.

What if we had testing library svelte specify resolve: { conditions: ['browser', 'node'] } and then updated use of requestAnimationFrame to check BROWSER && !NODE?

@abdel-17
Copy link
Author

Why was this issue closed? It still hasn't been fixed in the latest Svelte version.

Screenshot 2024-10-31 at 10 44 28 AM

@dummdidumm
Copy link
Member

Yeah not sure why this was marked as closed, we need to document the alias/browser version thing - did you adjust that in your config?

@dummdidumm dummdidumm reopened this Oct 31, 2024
@abdel-17
Copy link
Author

abdel-17 commented Oct 31, 2024

Yeah it works now. I would also like to point out that it's not very clear in the docs that testing runic code requires the JSDom environment. I tried wrapping code that uses $derived and $effect in an $effect.root like the docs suggested, only to find out it noops everything inside and tests nothing because it gets removed from the build.

https://svelte.dev/docs/svelte/testing

JSDom was mentioned in the section for component testing, so I had thought it was not necessary.

EDIT:
Here is an example for clarification

Screenshot 2024-10-31 at 11 01 09 AM

@dummdidumm
Copy link
Member

to be clear you don't need a JSDOM environment, you need a browser environment.

dummdidumm added a commit that referenced this issue Nov 1, 2024
The browser condition is also necessary to test runes, so it makes sense to add it to the first occurence to the vite config. Also add a note about more fine-grained alias conditions.

Closes #13961
Rich-Harris added a commit that referenced this issue Nov 1, 2024
* docs: note browser condition earlier

The browser condition is also necessary to test runes, so it makes sense to add it to the first occurence to the vite config. Also add a note about more fine-grained alias conditions.

Closes #13961

* Update documentation/docs/07-misc/02-testing.md

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants