Skip to content
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

fix: Make lookup result relative to referrer #40

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Jul 21, 2022

Discovered while looking into denoland/deno#15266. The current import-map-relative design could pass as intentional, but that wasn't the way it was being used in cli and I think adjusting to the passed referrer makes more sense.

Also account for conflicts in lookup_imports(). Choose the least specific matching prefix key, rewrite the specifier with it, make sure the rewritten specifier isn't overrided by some other more specific prefix key. e.g.

// import_map.json
{
  "imports": {
    "a/": "https://foo.com/",
    "a/b/": "https://bar.com/"
  }
}

import_map.lookup("https://foo.com/b/c") should match the first entry, rewrite it to "a/b/c", realise that this conflicts with the second entry (it would be remapped to https://bar.com/c) and discard it.

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2022

CLA assistant check
All committers have signed the CLA.

@nayeemrmn
Copy link
Collaborator Author

This (reverse) lookup API doesn't belong in this repo IMO, we should keep the implementation in the LSP. It's non-spec and needs to have opinionated selection logic specific to the application i.e. the import-map-remap diagnostic.

src/lib.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator Author

@dsherret Can this be landed?

@nayeemrmn nayeemrmn closed this May 4, 2023
@bartlomieju
Copy link
Member

Nayeem, I would actually be interested in landing this PR - I want to finally solve that issue in the LSP where you get a completion for a symbol and it doesn't use import map prefix but the full URL. I was thinking we should just remap the specifiers provided from TSC to use import map (if they're eligible) and it appears this API would be perfect for that purpose.

Could we reopen this PR?

@nayeemrmn nayeemrmn reopened this May 17, 2023
@bartlomieju
Copy link
Member

@nayeemrmn could you add your example from the PR description as a test?

Comment on lines +363 to +367
let specifier = Url::parse("https://foo.com/b/c").unwrap();
let result = import_map.lookup(&specifier, &referrer);
// The specifier should be not be reverse-mapped by the first entry, because
// it would be re-mapped somewhere else by the second entry.
assert_eq!(result, None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I'm a little lost here - why isn't it getting remapped by the first entry? There is no other entry that could remap it to something else, since one entry is for foo.com and the other is for bar.com

Copy link
Collaborator Author

@nayeemrmn nayeemrmn May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was successfully reverse-mapped by the first entry, that output would be forward-mapped by the second entry to something that wasn't the original URL, so import_map.resolve(import_map.lookup("https://foo.com/b/c")) == "https://foo.com/b/c" wouldn't hold.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how would that happen if the first entry can't remap the provided URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're asking why the reverse-lookup output a/b/c wouldn't get remapped by the first entry. It's because the second entry also matches it and would take precedence because it's more specific. So we have to prevent that reverse-lookup from succeeding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I "almost" understand this test. Is it a mistake in the test that we're testing https://foo.com/b/c? If it was https://foo.com/a/b I would understand your train of thought but right now I'm lost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No https://foo.com/ will be substituted by the a/.

Let's put it like this. If we didn't have this conflict-check, someone has deno.json like:

{
  "imports": {
    "a/": "https://foo.com/",
    "a/b/": "https://bar.com/"
  }
}

and the code

import "https://foo.com/b/c";

the LSP would recommend they abbreviate that to:

import "a/b/c";

What will that resolve to on deno run? Because of the more specific second entry it will be:

import "https://bar.com/c";

which isn't the intended target.

So this PR precisely detects those conflicts.

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.

None yet

4 participants