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

Merging behavior: should we merge within scopes? #153

Open
domenic opened this issue Jul 11, 2019 · 7 comments
Open

Merging behavior: should we merge within scopes? #153

domenic opened this issue Jul 11, 2019 · 7 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Jul 11, 2019

(Previously the general topic was discussed in #88, but opening a new issue for this specific problem.)

The current spec defines merging very simply. Basically, you have

const mergedImportMap = {
  imports: { ...importMap1.imports, ...importMap2.imports },
  scopes: { ...importMap1.scopes, ...importMap2.scopes }
};

In particular, this means that any scope definitions in the second import map completely override the first, as shown in the example linked in the spec.

In #150 @hiroshige-g pointed out that another potential design would be to always merge at the specifier map level. That would mean that if you have the same scope defined in two import maps, the contents of that scope get merged, instead of the second scope definition overriding the first.

I can see the attraction of such a design. I am OK switching, but would like community feedback.

My only worry is that I really do not want to produce a more complex merging behavior. (For example, merging address arrays.) The current design is intentionally very simple and easy to model. I worry that by introducing something with a bit more complexity, we'll be making users expect that merging will be "smart" and "do what I mean", which is a target I do not want to aim for.

Thoughts? We should decide this soon.

For clarity let's discuss the alternatives as top-level only merge behavior or merge-within-scopes behavior.

@domenic domenic added this to the MVP milestone Jul 11, 2019
@guybedford
Copy link
Collaborator

guybedford commented Jul 11, 2019

As an additional point on merging - is { "scopes": { "x": null } } supported for nulling out previously defined scopes? I guess { "scopes": { "x": {} } } can work just as well here though.

@domenic
Copy link
Collaborator Author

domenic commented Jul 11, 2019

You gotta use {}; any non-object will throw.

What that code will do, though, is an interesting question. In the top-level merge behavior, it will null out the previously-defined scope. In the merge-within-scopes behavior, it will be a no-op. (Similar to how you can't null out the top-level "imports" via a second import map with { "imports": {} }.)

@domenic
Copy link
Collaborator Author

domenic commented Jul 22, 2019

Let me drop in a concrete example to try to make this issue more approachable. Given:

<script type="importmap">
{
  "imports": {
    "a": "/a-1.mjs",
    "b": "/b-1.mjs",
    "std:kv-storage": ["std:kv-storage", "/kvs-1.mjs"]
  },
  "scopes": {
    "/scope1/": {
      "a": "/a-2.mjs"
    }
  }
}
</script>
<script type="importmap">
{
  "imports": {
    "b": null,
    "std:kv-storage": "kvs-2.mjs"
  },
  "scopes": {
    "/scope1/": {
      "b": "/b-2.mjs"
    }
  }
}
</script>

The top-level only merge (current spec) gives

<script type="importmap">
{
  "imports": {
    "a": "/a-1.mjs",
    "b": null,
    "std:kv-storage": "kvs-2.mjs"
  },
  "scopes": {
    "/scope1/": {
      "b": "/b-2.mjs"
    }
  }
}
</script>

whereas the merge-within-scopes behavior gives

<script type="importmap">
{
  "imports": {
    "a": "/a-1.mjs",
    "b": null,
    "std:kv-storage": "kvs-2.mjs"
  },
  "scopes": {
    "/scope1/": {
      "a": "/a-2.mjs",
      "b": "/b-2.mjs"
    }
  }
}
</script>

@bakkot
Copy link

bakkot commented Aug 15, 2019

In #167 we chose merge-within-scopes; it seems natural to use the same process to merge each individual same-scoped mapping as is used for the top-level one.

@frank-dspeed

This comment was marked as off-topic.

@trusktr

This comment was marked as off-topic.

@lemanschik

This comment was marked as off-topic.

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

No branches or pull requests

6 participants