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

Compatibility with SystemJS dynamic import maps #42

Closed
mkampmey opened this issue Sep 28, 2020 · 9 comments
Closed

Compatibility with SystemJS dynamic import maps #42

mkampmey opened this issue Sep 28, 2020 · 9 comments

Comments

@mkampmey
Copy link

I am using the dynamic import maps plugin for SystemJS because I need to slightly modify the content of my importmap.json file after it was downloaded. So my code looks somehow like this:

document.addEventListener('DOMContentLoaded', function () {
  fetch('/importmap.json')
      .then(function(response) {
        response.json().then(function(importMap) {
          for (var module in importMap) {
            if (importMap.hasOwnProperty(module)) {
              importMap[module] = changeUrl(importMap[module])
            }
          }
          var importmapElem = document.createElement('script');
          importmapElem.type = 'systemjs-importmap';
          importmapElem.innerHTML = JSON.stringify(importMap, null, 2);
          document.head.appendChild(importmapElem);
        })
      })
  });
})

I also inserted the webcomponent import-map-overrides-full so that I can modify the import map via the UI. Now, the problem is that no overrides have any effect. If I use a static script block for the import map, everythings works as expected. Is there anything I can do to be compatible with the dynamic import maps plugin?

@joeldenning
Copy link
Member

joeldenning commented Sep 28, 2020

Hmm this is something I'd like to support, but am not sure what the best implementation approach is. The issue is related to which import map takes precedence when multiple are supported.

The import maps spec used to say that the ordering/priority of import maps should be based on when they are inserted into the dom. This means that a dynamically inserted import map would always have top priority, even if it is placed into the dom so that it's before the other import maps. You can read more about this at WICG/import-maps#114

That part of the spec was removed when they dropped support for multiple import maps in v1 of the spec. SystemJS does not (and never did) implement that part of the spec, but has always relied on the order in the DOM (not insertion order) to determine priority. To see how systemjs relies on DOM order, see https://github.com/systemjs/systemjs/blob/42ab883c01c1ea2e17d000061f7f73bcfaba6558/src/features/import-maps.js#L28 (note that querySelectorAll returns a NodeList in dom order).

What this means is that in order to support dynamic import maps inside of import-map-overrides, we'd need to ensure that the override import map is always last in the dom, so that it has highest priority. Alternatively, the dynamic import maps could be intentionally inserted before the override map, to preserve override functionality.

Here's a workaround that should work for you, without waiting on any changes to import-map-overrides:

const dynamicMap = {imports: {foo: '/foo.js'}}
const dynamicMapElement = document.createElement('script')
dynamicMapElement.type = 'systemjs-importmap'
dynamicMapElement.textContent = JSON.stringify(dynamicMap)
insertDynamicMap(dynamicMapElement)

function insertDynamicMap(dynamicMapElement) {
  const overrideMapElement = document.querySelector('[data-is-importmap-override]')
  if (overrideMapElement) {
    overrideMap.parentNode.insertBefore(dynamicMapElement, overrideMapElement)
  } else {
    document.head.appendChild(dynamicMapElement)
  }
}

Let me know if that works for you. We could also add some logic to import-map-overrides that sets up a Mutation Observer that moves the overrideMapElement to always be the last map on the page. However, I'm not sure if there's a way we could guarantee that the mutation observer in import-map-overrides would execute before the systemjs mutation observer (see here). An alternative might be to hook the prepareImport hook (docs) to ensure correct ordering there. I actually like that approach and might do it that way:

https://github.com/systemjs/systemjs/blob/master/docs/hooks.md#prepareimport---promise

@joeldenning
Copy link
Member

I spent some time exploring the prepareImport() solution. It works except for in the case where import-map-overrides is loaded on the page before systemjs. If we can find a way of patching the prepareImport correctly even when SystemJS has not yet been loaded on the page, then I'd be happy to add this to import-map-overrides. Otherwise, we might need to explore other options for solving this.

Here's my wip branch: https://github.com/joeldenning/import-map-overrides/tree/issue-42

@mkampmey
Copy link
Author

Big thanks for your reply! I didn't know that the priority is based on the order of the import maps in the dom. I tried the workaround you mentioned but unfortunately it does not work, even though the override import map is last in the dom.
I didn't spent more time on this because I checked the SystemJS hooks and the resolve hook is exactly what I need. With that hook I can change the url in the import map without the need for the dynamic import maps plugin. :)

@joeldenning
Copy link
Member

Glad to hear it. I'll keep this issue open for future users who may be searching for support with dynamic import maps.

@maheshramaiah
Copy link

@joeldenning, Did the code snippet you shared in the above comment by inserting the dynamic import map before the override map worked for you ? I have such an use case and i tried the solution mentioned in code snippet, it didn't worked.

@joeldenning
Copy link
Member

I didn't test it - just wrote it out in the Github issue and asked the original reporter to test it. Could you clarify what part didn't work?

@maheshramaiah
Copy link

maheshramaiah commented Jun 8, 2021

I have been creating import maps dynamically, and as you suggested i am appending the import map before the overridable import map, but when i am overriding the dynamically created import map through the override UI, there is no change, it is still referring to the default import map that was created dynamically.
Here is code snippet i am using for creating dynamic import map -

const overrideMapElement = document.querySelector('[data-is-importmap-override]');
const script = document.createElement('script');

script.type = 'systemjs-importmap';
script.textContent = JSON.stringify( { imports: { foo: '/foo.js' }});

if (overrideMapElement) {
   overrideMapElement.parentNode.insertBefore(script, overrideMapElement);
} else {
   document.head.appendChild(script);
}

@maheshramaiah
Copy link

I was able to solve the issue in a bit different manner, not sure if this is recommended and suitable for all.
Before inserting the import maps, i did a filter on already inserted systemjs-importmaps and ignore adding it again. Since, overridable would have already created systemjs-imports maps from local storage with overridable maps, the dynamic import map injection from code will get ignored and systemjs will resolve module from overridable map. This solution works me, because i would not expect the import url to change once inserted.

But i would still like to know, what was the issue with code snippet i used, since systemjs relies on order of import maps in the DOM for resolution, but somehow systemjs resolved the import with older url.

@joeldenning
Copy link
Member

joeldenning commented Jun 11, 2021

I created https://codesandbox.io/s/funny-snyder-x1sdc?file=/index.html which shows this. I confirmed that my code snippet in this github issue is working as I expected, but that SystemJS is behaving differently than I expected. It turns out that systemjs skips processing import maps that it has already processed. In other words, it prioritizes newly inserted maps over previous maps, regardless of DOM order. To understand why it does this, see WICG/import-maps#114. I didn't realize this when I shared the code snippet above, so that code is flawed and won't work.

See the following code in systemjs that shows this behavior:

https://github.com/systemjs/systemjs/blob/75853dddde25b13244059babc2657a60196c1b13/src/features/import-maps.js#L29-L31

https://github.com/systemjs/systemjs/blob/75853dddde25b13244059babc2657a60196c1b13/src/features/import-maps.js#L48

With that in mind, I think the best approach is just to reinsert the override map after the first dynamic map. Here's some code and a code sandbox that shows how:

https://codesandbox.io/s/headless-field-grkik?file=/index.html:1424-1956

// insert a new import map
insertImportMap({
  imports: { a: "/a2.js", b: "/b2.js" }
});
// reinsert the override map, to force systemjs to reprocess it.
insertImportMap(importMapOverrides.getOverrideMap());

System.prepareImport(true).then(() => {
  console.log('new import maps applied!')
});

function insertImportMap(importMap) {
  const script = document.createElement("script");
  script.type = "systemjs-importmap";
  script.textContent = JSON.stringify(importMap);

  document.head.appendChild(script);
}

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

3 participants