false positive unused variable warning

benmccann commented 4 months ago

Describe the bug

When building the popular immich photo management app, a warning is spit out:

"omit" is imported from external module "lodash-es" but never used in "src/lib/components/elements/dropdown.svelte", "src/lib/components/admin-page/settings/admin-settings.svelte", "src/lib/stores/assets.store.ts", "src/lib/utils/timeline-util.ts", "src/lib/components/admin-page/settings/job-settings/job-settings.svelte", "src/lib/components/admin-page/settings/machine-learning-settings/machine-learning-settings.svelte", "src/lib/components/admin-page/settings/map-settings/map-settings.svelte", "src/lib/components/admin-page/settings/thumbnail/thumbnail-settings.svelte", "src/lib/components/admin-page/settings/server/server-settings.svelte", "src/lib/components/admin-page/settings/trash-settings/trash-settings.svelte", "src/lib/components/admin-page/settings/new-version-check-settings/new-version-check-settings.svelte", "src/lib/components/admin-page/settings/library-settings/library-settings.svelte", "src/lib/components/admin-page/settings/logging-settings/logging-settings.svelte", "src/lib/components/admin-page/settings/ffmpeg/ffmpeg-settings.svelte", "src/lib/components/admin-page/settings/password-login/password-login-settings.svelte", "src/lib/components/admin-page/settings/oauth/oauth-settings.svelte", "src/lib/components/admin-page/settings/storage-template/storage-template-settings.svelte", "src/lib/components/admin-page/settings/theme/theme-settings.svelte", "src/routes/(user)/albums/+page.svelte" and "src/routes/(user)/map/+page.svelte".

However, this method is not imported in any of the files mentioned.

And it is indeed used in the one file where it is imported: https://github.com/immich-app/immich/blob/dca1bd22df067104129ca4f30274e4f8c9f3a349/web/src/routes/(user)/map/%2Bpage.svelte#L127

Reproduction

git@github.com:immich-app/immich.git

Steps to reproduce

git clone git@github.com:immich-app/immich.git
cd immich/open-api && git reset --hard dca1bd22df067104129ca4f30274e4f8c9f3a349 && ./bin/generate-open-api.sh
cd ../web && npm install
npm run build

System Info

Vite 5.1.1

Used Package Manager

npm

Logs

No response

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

sapphi-red commented 4 months ago

The warning is coming from rollup. https://github.com/rollup/rollup/blob/762420860765e8e46e24d48b38f5b98ca31735fa/src/utils/logs.ts#L1036-L1053

However, this method is not imported in any of the files mentioned.

The list contains src/routes/(user)/map/+page.svelte and that file does import omit.

My guess is that this part of the code will be removed in SSR, but the import will remain as-is, resulting in this warning.

benmccann commented 4 months ago

My guess is that this part of the code will be removed in SSR, but the import will remain as-is, resulting in this warning.

That's a very good guess. on: is an event handler and so it's quite possible that wouldn't get compiled to SSR.

They were able to workaround the warning in https://github.com/immich-app/immich/pull/7035, which replaced the omit import with a locally declared version of omit. I guess Vite must only have a warning for unused imports and not unused variables, which is what made that go away.

I wonder if Vite should not warn about unused imports. As long as it can tree-shake it then it's okay. And we can have eslint check for this which would handle it correctly.

Although, at the same time, we should test what happens in Svelte 5. The on: syntax from Svelte 4 is going away, so perhaps this isn't an issue as people migrate to the newer version

sapphi-red commented 4 months ago

I don't think Vite should behave differently from Rollup. (It seems the rational is written in https://github.com/rollup/rollup/issues/595, https://github.com/rollup/rollup/issues/1381)

Reading rollup/rollup#2124, it appears that rollup does not print the warning if the variable usage is tree-shaken away. Since the plugin removes the variable usage, rollup assumes that the variable was mistakenly imported.

vitejs / vite