knip does not detect type imports that happen in svelte files

[thenbe]

In the current svelte compiler recipe, we compile a svelte file straight to javascript, then pass the javascript to knip. As a result, knip never sees type imports that happen in svelte files. For example:

// myfile.ts
export type Foo = any
export type Bar = any
export const baz = ''

<script lang='ts'>
  import type { Foo } from './myfile'
  import { type Bar } from './myfile'
  import { baz } from './myfile'
</script>

Here, knip will report both Foo and Bar as "unused type exports" (assuming no other typescript file also imports them).

[thenbe]

So after working on this and coming up with this workaround (#411) to fix the type import detection, I decided to try the regex recipe (identical to the one suggested for vue) in my svelte project. And it... worked perfectly. Granted, I did not test it thoroughly, but knip behaved as expected at an initial glance at least.

I initially did not try this regex approach because the docs did not explicitly suggest it for svelte. Maybe it should? That way, users can start with that simple approach and escalate as they see fit?

Either way, users should know that the current suggested knip compiler recipe for svelte recipe does not detect type imports. Known alternatives that do, include:

1. the workaround in #411
2. the regex approach (similar to what is currently suggested for vue. Although it might need more testing.)

[webpro]

Just to make sure we're on the same page here, this is the Vue compiler from the docs:

const compiler = /<script\b[^>]*>([\s\S]*?)<\/script>/gm;

export default {
  compilers: {
    vue: text => {
      const scripts = [];
      let match;
      while ((match = compiler.exec(text))) scripts.push(match[1]);
      return scripts.join(';');
    },
  },
};

This returns all contents of <script> tags, but does not get rid of the export modifier for props as you've shown in this example:

    svelte: async (text: string) => {
      const js = await compileSvelteCode(text);
      let ts = await extractScriptTS(text);
      // Remove `export` modifiers since these are props, not real exports.
      ts = ts
        .replace(/export let/g, 'let')
        .replace(/export const/g, 'const')
        .replace(/export class/g, 'class')
        .replace(/export function/g, 'function');
      return `${ts}\n${js}`;
    },

So I'm inclined to think the Vue regex-based example misses that part and would be incomplete cq result in false positives?

[thenbe]

Yup you're correct. I failed to mention that we still need to strip away any export keywords.

A full example would be something like:

const compiler = /<script\b[^>]*>([\s\S]*?)<\/script>/gm;

export default {
    paths: {
        '$app/*': ['node_modules/@sveltejs/kit/src/runtime/app/*'],
        '$env/*': ['.svelte-kit/ambient.d.ts']
    },
    compilers: {
        svelte: async (text: string) => {
            const scripts = [];
            let match;
            while ((match = compiler.exec(text))) scripts.push(match[1]);
            let script = scripts.join(';');
            script = script
                .replace(/export let/g, 'let')
                .replace(/export const/g, 'const')
                .replace(/export class/g, 'class')
                .replace(/export function/g, 'function');
            return script;
        },
        css: (text: string) => [...text.matchAll(/(?<=@)import[^;]+/g)].join('\n')
    }
};

[webpro]

I'm working on auto-installing the compilers in the v4 branch.

Any chance you could try v4.0.0-canary.7 on some of your workload? I.e. the compilers for the extensions as listed in the examples can be removed from the knip.ts configuration :)

I've tried it myself in the fixtures, in Knip docs itself (Astro) and on the https://github.com/TanStack/query repo, looking OK so far. If anything, each compiler can be overridden if there are any issues (or old/new version of framework needs to be supported, etc). This means no breaking changes, but I've still done it in v4 because the internal changes are pretty significant and it's also more clear documentation-wise (haven't updated those yet btw).

You can install using e.g. npm install -D knip@canary

[thenbe]

Ran it on a couple of projects and it behaves as expected; I was able to remove the svelte key (config.compilers.svelte) and knip would continue to report accurate results. Also ran it on a demo sveltekit project (npm create svelte@latest myapp) and was able to reduce the config to:

- const compiler = /<script\b[^>]*>([\s\S]*?)<\/script>/gm;
-
  export default {
    paths: {
      '$app/*': ['node_modules/@sveltejs/kit/src/runtime/app/*'],
      '$env/*': ['.svelte-kit/ambient.d.ts'],
    },
    compilers: {
-     svelte: async (text: string) => {
-       const scripts = [];
-       let match;
-       while ((match = compiler.exec(text))) scripts.push(match[1]);
-       let script = scripts.join(';');
-       script = script
-         .replace(/export let/g, 'let')
-         .replace(/export const/g, 'const')
-         .replace(/export class/g, 'class')
-         .replace(/export function/g, 'function');
-       return script;
-     },
      css: (text: string) => [...text.matchAll(/(?<=@)import[^;]+/g)].join('\n')
    }
  };

I did have to leave the config.compilers.css key though. Otherwise if I removed it, knip would report @fontsource/fira-mono as an unused devDependency.

[webpro]

Awesome, thanks so much for trying!

Indeed I didn't add a CSS compiler yet, because the others have a nice and simple mechanism to conditionally enable them: the presence of a certain dependency. To be continued :)

[bleucitron]

Hey !

I've been struggling with knip and Svelte, just to find out this issue, which seems to solve my undetected imports issue.

FYI, the following might be irrelevant with the upcoming v4.0.0-canary.7 release, but it seems the current svelte recipe as it appears on the docs fails if you use :global in your styles:

/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:14237
    const error = new CompileError(message);
                  ^

CompileError [ParseError]: Identifier is expected
    at error (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:14237:16)
    at Parser.error (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:14385:3)
    at Object.read_style [as read] (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:10410:11)
    at tag (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:13052:27)
    at new Parser (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:14333:12)
    at parse (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:14510:17)
    at compile (/Users/Romain/Code/radiofrance/webrf/node_modules/svelte/compiler.cjs:45687:14)
    at compileSvelteCode (/Users/Romain/Code/radiofrance/webrf/knip.ts:33:42)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async svelte (/Users/Romain/Code/radiofrance/webrf/knip.ts:12:18) {
  code: 'css-syntax-error',
  start: { line: 172, column: 4, character: 5850 },
  end: { line: 172, column: 4, character: 5850 },
  pos: 5850,
  filename: undefined,
  frame: '170: \n' +
    '171:   .Home-layout {\n' +
    '172:     :global(&&& .layout-TopBanner > .banniere_haute) {\n' +
    '         ^\n' +
    '173:       margin: auto;\n' +
    '174:     }'
}

(Should i open an issue still ?)

[webpro]

Should i open an issue still ?

Even better: a pull request :)

[bleucitron]

I wouldn't know how to solve this myself, plus with v4.0.0-canary.7, the pb seems to be gone, probably because knip.ts does not execute the svelte compiler at all (using the example [here] provided (https://github.com/webpro/knip/issues/410#issuecomment-1877891160)), so i might be wrong, but i'm not sure this is necessary.

[webpro]

I think this should be alright in v4, let me know if there's still issues left.