Skip to content

fix: pre-optimize clsx #1067

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: pre-optimize clsx #1067

wants to merge 1 commit into from

Conversation

GauBen
Copy link

@GauBen GauBen commented Jan 14, 2025

Small fix to address these small log lines:

  VITE v6.0.7  ready in 1533 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help
11:33:51 PM [vite] (client) ✨ new dependencies optimized: clsx
11:33:51 PM [vite] (client) ✨ optimized dependencies changed. reloading

@bluwy
Copy link
Member

bluwy commented Jan 15, 2025

Do you have an example where this could happen? Svelte recently support using clsx internally, but it's part of its runtime and should already be optimized together.

@GauBen
Copy link
Author

GauBen commented Jan 15, 2025

Yep! Not really a minimal repro, but running

git clone https://github.com/GauBen/gautier.dev
cd gautier.dev
yarn
yarn dev

Should display the following:

yarn dev
Forced re-optimization of dependencies

  VITE v6.0.7  ready in 795 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help
{
  message: 'Bad credentials',
  documentation_url: 'https://docs.github.com/graphql',
  status: '401'
}
9:42:46 AM [vite] (client) ✨ new dependencies optimized: clsx
9:42:46 AM [vite] (client) ✨ optimized dependencies changed. reloading
{
  message: 'Bad credentials',
  documentation_url: 'https://docs.github.com/graphql',
  status: '401'
}

@dominikg
Copy link
Member

only seems to happen with vite6

https://www.sveltelab.dev/r13rocirxc06ybv

@dominikg
Copy link
Member

https://svelte.dev/playground/hello-world?version=5.19.3#H4sIAAAAAAAAA22OywrCMBBFfyXOJgrF0m2MBXf-Q9NF2k5pISYhGV-E_LtEETduD-dwbwKrLwgCzmiMY3cXzMS2OK2E0w4qmFeDEUSXgJ6-eAVA9a1O3u_jDQ0VNuiI__joLKGlCAJkHMPqqVVWkUFiRWdHxt-7_KCsrH-GlUvDRqNjPKaOz87xig868D63n7ep5Hkj66VplYUKCB8EgsIVc59fpACa9tsAAAA=

import 'svelte/internal/disclose-version';
import 'svelte/internal/flags/legacy';
import * as $ from 'svelte/internal/client';

var root = $.template(`<h1></h1>`);

export default function App($$anchor) {
	let name = 'world';
	var h1 = root();

	$.set_class(h1, $.clsx(['foo', 'bar']));
	h1.textContent = `Hello ${name ?? ''}!`;
	$.append($$anchor, h1);
}

svelte generates code that imports clsx, ultimately it is imported here:
https://github.com/sveltejs/svelte/blob/357e1a74b4af25c3e3839d303cc3d8d239d43087/packages/svelte/src/internal/shared/attributes.js#L2

i wonder how vite5 is able to detect this eagerly.

clsx was only added in a later version of svelte5, so to avoid issues with earlier versions, the inject would have to check if clsx is a dependency of svelte. But ideally we find out why vite6 behaves differently (or if the same can happen in vite5 and it was just my testing that was off).

@dominikg
Copy link
Member

had another look, whats even more puzzling is that clsx doesn't need to be optimized, it is not using "type": "module" but it does export esm with a single .mjs file.

  "exports": {

    ".": {

      "import": {

        "types": "./clsx.d.mts",

        "default": "./dist/clsx.mjs"

      },

so adding this to optimizeDeps.include is really weird. i wonder how it is determined that it needs optimization in the first place. This feels like a vite issue the more i think about it.

@dominikg
Copy link
Member

if we were to add it, i think it would have to be in the form svelte > clsx and we would need a testcase that checks it somehow. we do have one for optimizeDeps.include generation but that doesn't cover the "no manual optimizations after the fact" case iirc

@dominikg
Copy link
Member

because it does export esm and is single file anyways we could also put it in optimizeDeps.exclude as optimizing it won't do anything of value.

@dominikg
Copy link
Member

unfortunately excluding does not work because vite import analysis then can't find it in the optimized svelte. 😞

@GauBen
Copy link
Author

GauBen commented Apr 22, 2025

Be careful, it looks like you fell into a rabbit hole 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants