-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add spec for "update an import map" #150
Conversation
spec.bs
Outdated
1. Assert: |import map| is not null. | ||
1. Assert: |new import map| is not null. | ||
1. <p class="note">TODO: Implement merging. This merges |new import map| into |import map| and thus updates |import map| in-place.</p> | ||
1. For each |specifier| → |addresses| of |newImportMap|'s [=import map/imports=], [=map/set=] |importMap|[|specifier|] to |addresses|. |
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.
|importMap|[|specifier|] and |importMap|[|url|] should be something like:
|importMap|'s scopes[|specifier|]
|importMap|'s imports[|url|]
</div> | ||
|
||
<div class="example" id="example-merging"> | ||
[=Update an import map=] merges the two import maps in a very simple way, not performing any deep merging beyond the top level of the "`imports`" and "`scopes`" keys. For example, |
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.
Although I feel a little inconsistent (because the specifier map
as import map
's imports
is merged while specifier map
s inside import map
's scopes
are overwritten, not merged), basically this behavior is fine with me.
If there are any discussions / analyses behind this decision, it will be helpful to reference them.
(In any cases, there might be still "unexpected" behavior around merging, because parts of different import maps can be applied to different parts of a single module script, but so far I have no idea for such kind of unexpected behavior, and probably we have to wait for user feedbacks.)
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.
Not too much besides just a general drive for simplicity. I would be OK with slightly deeper merging. I will open an issue to discuss, but we should decide soon.
@hiroshige-g to review.
We'll need tests for this eventually, especially the sorting behavior and how that effects precedence, but I figured it'd be more straightforward to do those as web platform integration tests as part of the Chrome implementation, instead of doing them in the reference implementation.
Preview | Diff