denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.9k stars 5.22k forks source link

[lsp] `deno.jsonc` option to disable `import-map-remap` #17867

Open loynoir opened 1 year ago

loynoir commented 1 year ago

Related https://github.com/denoland/deno/issues/15266

I tried // deno-lint-ignore import-map-remap, seems not working.

petamoriken commented 1 year ago

// deno-lint-ignore comment applies to deno_lint rules. https://lint.deno.land/

loynoir commented 1 year ago

@petamoriken Yes, seesm there is no option to let the misbehave import-map-remap gone.

dsherret commented 1 year ago

This is a deno diagnostic and not a deno lint diagnostic. I don't think we should have an option for this, but rather fix the underlying issue with the diagnostic or remove it.

nayeemrmn commented 1 year ago

but rather fix the underlying issue with the diagnostic or remove it.

I could revive #15267 which covers a lot of undesired cases, but I think it would be better to remove it. With certain types of import maps (relative-to-relative) it's impossible to know if the mapped or unmapped variant is preferred. And the 'reverse lookup' functionality it depends on is non-spec, potentially impossible to define correctly and would always be error prone. I don't think people are looking for this feature either. It was specifically implemented as a hint for the first reason, but it's been shown that people still treat that as a nagging error.

Same with https://github.com/denoland/import_map/pull/40, rather than fixing it we should remove the non-standard and error prone ImportMap::lookup() from such an otherwise spec-guided crate.

lucacasonato commented 1 year ago

@nayeemrmn We should fix this feature, not remove it. If I am inside of a Fresh project, and I want to import useState, I want to import it from preact/hooks, not https://esm.sh/~/preact/2187312i3jbnjh237dh172y3h217sdj1238jkd/hooks/index.js?foo=bar32. I consider this is the #1 issue with the LSP at this moment.

The reverse lookup functionality in ImportMap should be improved, not removed.

nayeemrmn commented 1 year ago

I guess I'm struggling to understand the workflow surrounding it. The diagnostic and code action come into effect if you input that URL in the first place, I speculated it wasn't useful because users would just start with preact/hooks if they configured it. Is the URL the result of an autocomplete?

bartlomieju commented 1 year ago

Is the URL the result of an autocomplete?

Yes, if you have an import map and you select suggested autocomplete the LSP will put the full URL instead of the import mapped specifier.

I believe this is covered by https://github.com/denoland/deno/issues/15330

alex-kinokon commented 1 year ago

I have "~/": "./" in my importmap.json and the LSP thinks I want to change ./fresh.gen.ts to ~/fresh.gen.ts and ./Button.tsx to ~/src/components/Button.tsx. So if you use project root for absolute imports as suggested by the official manual, it’s basically impossible to do any relative imports without triggering this problem.

spence commented 10 months ago

20539 doesn't seem to fix this issue. example:

image

i would love 🙏 if we could disable the squiggle.

bradthomasbrown commented 3 weeks ago

apparently, the squiggle issue doesn't seem to appear if the import is in a type expression. so if you want to remove it, you can write this (very messed up) workaround expression

await import("../value.ts" as string) as typeof import("../value.ts")

this removes the squiggle from the actual import expression by turning it into a dynamic import, then asserts the type to "put the type back"