lsp: import-map-remap misbehaves with relative map keys

[baoshan]

After upgrading to v1.24.0, importing from local files (as long as the module is not in the root directory) triggers below warning:

The import specifier can be remapped to "./some_dir/bar.ts" which will resolve it via the active import map. deno(import-map-remap)

[bartlomieju]

cc @kitsonk

[nayeemrmn]

There are two parts to this,

• ImportMap::lookup() doesn't account for relative map keys, and returns values that are relative to the import map rather than the referrer.
• The diagnostic should only be given when the suggested map key is a bare specifier. Applying it to relative specifiers is too opinionated as to which of the alternative import forms is preferred, and there can be conflicts/overlaps in the map key scopes that are too complicated to account for. Edit: I think I came up with good conflict resolution in https://github.com/denoland/import_map/pull/40. But nevertheless.

[kitsonk]

I disagree that it should only be applied to bare specifiers. If you have remapped stuff locally in an import map, having a hint diagnostic and a quick fix doesn't hurt anything.

@lucacasonato thoughts?

[lucacasonato]

I also agree there is a problem, but I couldn't pin point a good way to fix it off the cuff. For me, this makes it much more frustrating to work in the fresh repo itself (but makes it nicer to use in fresh projects). Just open https://github.com/denoland/fresh and open some of the files in src/. You'll see annoying squiggly lines pretty quickly.

[lucacasonato]

Maybe we could just remove the squiggly line, but keep the quick fix?

[baoshan]

The quick fix currently suggests replacing ./bar.ts with ./utils/bar.ts, which does not look good to me.

[nayeemrmn]

If you have remapped stuff locally in an import map, having a hint diagnostic and a quick fix doesn't hurt anything.

Mappings between two non-bare URLs are niche and are typically used to bridge compatibility for modules you can't control. There's no reason to make suggestions for those.

An exception is the "/": "./", "./": "./" pattern like @baoshan is using. But as they mentioned on Discord, ./bar.ts doesn't need to be replaced with /bar.ts. The preferred import form will be very opinionated.

[kitsonk]

An exception is the "/": "./", "./": "./" pattern like @baoshan is using. But as they mentioned on Discord, ./bar.ts doesn't need to be replaced with /bar.ts. The preferred import form will be very opinionated.

Yes, but that is why the diagnostics are flagged as hints, indicating they can be updated.

The purpose and intent of flagging them for relative imports is for example if you are developing/refactoring code where you have moved your dependencies into a vendor directory and you have an import applied that resolves to those. The quick fix allows you to move quickly to bare specifiers.

Considering that, I can see how ./file.ts or ./subdir/file.ts might get annoying (but again, that import map is written to resolve there, it wouldn't provide the suggestions unless the import map resolves there), but retain them for ../ specifiers and root (/) specifiers.

Maybe we could just remove the squiggly line, but keep the quick fix?

I can look into that, but the diagnostics were already set at the "lowest" priority and I think you need a diagnostic to be able to apply a quick fix. Refactorings are a different story, and I could look into see if doing it as a refactoring might be better all around.

[nayeemrmn]

The quick fix allows you to move quickly to bare specifiers.

My proposed fix doesn't disable the diagnostic if the quick fix is a bare specifier, it only disables cases where one relative specifier would otherwise be suggested in place of another.

[ghost]

I came here looking for a fix to remove the little ellipsis [0] that indicates a suggestion for my relative imports, when using an import map.

My issue with it is that I don't want to use the import map in all cases, usually only when I'm importing from a higher directory.

[0] ellipsis shown beneath the first double-quote after the word from in an import:

[jollytoad]

I've been enjoying the import-map remapping feature immensely since it arrived, it was a right PITA manually adjusting them before.

I thought I'd drop a use-case in here for consideration, I'd hate to lose remapping in my case just because it doesn't align with someone else's expectations of it.

I have an import map that maps some top-level folders to bare-specifiers as such...

{
    "imports": {
        "$app/": "./app/",
        "$lib/": "./lib/"
    }
}

So I have a /lib folder containing folders that you could consider as distinct packages, imported as such: $lib/something/mod.ts, $lib/other/foo.ts

VSCode will initially generate a relative import for these though, depending on depth: ../../lib/other/foo.ts ... and the remap feature will offer to correct that for me. In this case the import was in a different package.

But, if the module is in the same package, and a relative import is generated, ie: ../foo.ts or maybe ./foo.ts, it still suggests the remapping.

I want the remap behaviour for the first case but not necessarily the second.

I'm thinking that a sensible heuristic to handle this would be for the remapper to consider what the current module maps to, compared to the target import, and if they have the same base then remapping could be considered unnecessary.

For example, in module /lib/other/sub/thing.ts, we have import from ../foo.ts, so the remap can see that the module could map to $lib/other/sub/thing.ts and the import to $lib/other/foo.ts, so the base of the import ($lib/other/) matches that of the module, so don't suggest to remap.

Whereas, in module lib/something/mod.ts, we import from ../other/foo.ts, where module maps to $lib/something/mod.ts and the import maps to $lib/other/foo.ts, the base doesn't match, so it suggests the remapping.

I'm not sure if such a behaviour would suit all projects, and so should probably be opt-in initially. It may also not be desirable in test cases either, where you may want to always remap, even within the same package.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[sant123]

I'm curious if there is any progress on this? I'm following this blog steps and stopped in this part:

Note: you can add "/": "./", "@/": "./" to your import_map.json so that you can import from posts.ts with a path relative to root:

[sant123]

https://user-images.githubusercontent.com/7959437/222334779-2bb6ef86-5232-4edc-ae57-829c5f44d09c.mp4

[JetLua]

Is there any new progress on this issue?