-
Notifications
You must be signed in to change notification settings - Fork 5.7k
feat: multiple import maps #30754
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
base: main
Are you sure you want to change the base?
feat: multiple import maps #30754
Conversation
Given that multiple import maps are possible now, should --import-map argument be mutually exclusive with |
Note that the import_map loading process is not recursive right now, so it won't be possible to point importMap field to another workspaces deno.json, the initial focus is only on loading standard-compliant import maps, without nested importMap field. |
dependencies: maybe_external_import_maps | ||
.into_iter() | ||
.flat_map(deno_config::import_map::import_map_deps) | ||
.chain( | ||
root_folder | ||
.deno_json | ||
.as_deref() | ||
.map(|d| d.dependencies()) | ||
.unwrap_or_default(), | ||
) | ||
.collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged importmap approach won't work here either, since by spec the first importmap with resolved import takes precedence, yet in case of multiple jsr/npm imports - during the merge we might want to satisfy version requirements of all import maps here, and this is only possible if we'll be able to access all the referenced import maps.
c4234ad
to
f93c9b5
Compare
LSP resolver is broken, an I'm not sure right now how it should work in the first place. Resolver seem to work fine for other purposes, yet there is still no tests for updated functionality. |
Remaining LSP resolver test failures are caused by ImportMap::resolve method, which may resolve paths like pub fn resolve(
&self,
specifier: &str,
referrer: &Url,
) -> Result<Url, ImportMapError> {
let as_url: Option<Url> = try_url_like_specifier(specifier, referrer);
...
// The specifier was able to be turned into a URL, but wasn't remapped into anything.
if let Some(as_url) = as_url {
return Ok(as_url);
}
} Thus not triggering the "continue to the next import_map" logic. |
2851085
to
fa26b18
Compare
After the last commits there is no failing tests. Import map merging is used for lsp, but not in a way I wanted to. Still, I believe diagnostics and further improvements should be developed in later PRs. CI doesn't pass here, as I am adding workspace patch for my import_map PR (denoland/import_map#91), and despite it declared in .gitmodules - it seems that CI is not doing recursive clone, thus fails. |
Co-authored-by: David Sherret <[email protected]> Signed-off-by: Lach <[email protected]>
While in web you can have multiple import maps, Deno only allows one to be specified right now.
importMaps are merged at WorkspaceResolver level, and passed as a list everywhere else, as it is useful to look at all requirements sometimes, e.g on package installer level - resolver might want to take into account all the requirements, including overriden.
The current implementation lacks tests, especially around LSP part, as I haven't figured out some parts about the implementation that Deno team wants, e.g: right now deno disallows specifying both import map and imports in deno.json, yet this check seems to be redundant in case of multiple import maps present.
Fixes: #25450 #30689