-
Notifications
You must be signed in to change notification settings - Fork 14
Import map merging #91
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?
Conversation
371f084 to
75f8d48
Compare
| #[derive(Debug, Clone)] | ||
| pub struct SpecifierMap { | ||
| #[deprecated = "import map base_url doesn't work with merged import maps"] | ||
| base_url: Url, |
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.
It seems that base_url be better stored by the side of ImportMaps that are not merged on the application side.
|
|
||
| #[deprecated = "specifier map base_url doesn't work with merged import maps"] | ||
| #[allow(deprecated)] | ||
| pub fn append(&mut self, key: String, value: String) -> Result<(), String> { |
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.
...In that case, methods like this be better implemented on SpecifierMapBuilder struct
|
|
||
| #[deprecated = "specifier map base_url doesn't work with merged import maps"] | ||
| #[allow(deprecated)] | ||
| pub fn contains(&self, key: &str) -> bool { |
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.
Yet I have no idea what's the good place for this nethod, but it doesn't seem to be used anywhere
|
Might as well postpone deprecation of base_url and related methods, It's just feels wrong to have such api, because all of the deprecated methods doesn't seem to be fit for already constructed import map, especially with how merged and synthetic ones behave. In case of synthetic import maps, deno already had an error, because workspace synthetic import map had inherited import_url from either deno.json, or from importMap declared in deno.json/passed using --import-map. Second case is invalid, because all the relative paths seem to be rewritten incorrectly when I was testing, and now I'm always setting workspace deno.json, which may not exist, but which makes sense for workspace package resolution. |
My implementation of multiple import maps in deno (denoland/deno#30754) iterates over import maps to find matches. It is, however, unable to detect if
ImportMap::resolvehas returned an entry, that should be tried from next import map.This change allows to only return remapped paths.
This PR also implements import map merging as described by spec: https://html.spec.whatwg.org/multipage/webappapis.html#merge-existing-and-new-import-maps
I can split those changes if that would be better for review purposes
Ref: denoland/deno#30754 (comment)