es-module-shims icon indicating copy to clipboard operation
es-module-shims copied to clipboard

Scripts of type "module-shim" don't seem to have access to specifier defined in importmap

Open zachsa opened this issue 1 year ago • 24 comments

Hi Guy,

In attempting to write a es-module-shims-plugin to parse JSX, I'm finding that files imported via <script type="module-shim"> can't seem to resolve specifiers declared in a

Working

// ./jsx-app.js
export default 'hello world'

Ad then in the HTML file

<script type="importmap">
{
  "imports": {
    "react": "https://ga.jspm.io/npm:[email protected]/index.js"
  }
}
</script>

<script type="module-shim">
      import app from './jsx-app.js'
      console.log(app)
</script>

But this errors

// ./jsx-app.js
import React from 'react'
export default 'hello world'

(The HTML is the same), I get the following error:

es-module-shims.js:440 Uncaught Error: Unable to resolve specifier 'react' imported from http://localhost:3000/test/jsx-app.js
    at throwUnresolved (es-module-shims.js:440:11)
    at _resolve (es-module-shims.js:397:71)
    at es-module-shims.js:825:32
    at Array.map (<anonymous>)
    at es-module-shims.js:821:45

Changing the script type to "module", and I see that React can be resolved from the imported module (./jsx-app.js):

<script type="module">
      import app from './jsx-app.js'
      console.log(app)
</script>

Please let me know if this issue makes sense - I was expecting to be able to resolve specifiers from the scripts imported via type "module-shim"

zachsa avatar Jun 24 '23 05:06 zachsa

Changing the importmap to importmap-shim seems to give me access to the importmap specifiers in my module-shim imported app.

However there's definitely a race condition somewhere. things usually work, but sometimes I get a parse error:

es-module-shims.js:392 Uncaught Error: Parse error http://localhost:3000/test/jsx-app.js:3:38
    at o (es-module-shims.js:392:18904)
    at parse (es-module-shims.js:392:17133)
    at es-module-shims.js:809:18
o @ es-module-shims.js:392
parse @ es-module-shims.js:392
(anonymous) @ es-module-shims.js:809
await in (anonymous) (async)
getOrCreateLoad @ es-module-shims.js:796
(anonymous) @ es-module-shims.js:832
await in (anonymous) (async)
(anonymous) @ es-module-shims.js:821
Promise.then (async)
getOrCreateLoad @ es-module-shims.js:819
topLevelLoad @ es-module-shims.js:536
await in topLevelLoad (async)
processScript @ es-module-shims.js:923
processScriptsAndPreloads @ es-module-shims.js:847
(anonymous) @ es-module-shims.js:499
Promise.then (async)
(anonymous) @ es-module-shims.js:475
(anonymous) @ es-module-shims.js:2

jsx-app.js:3 Uncaught SyntaxError: Unexpected token '<' (at jsx-app.js:3:16)
Promise.catch (async)
processScript @ es-module-shims.js:931
processScriptsAndPreloads @ es-module-shims.js:847
(anonymous) @ es-module-shims.js:499
Promise.then (async)
(anonymous) @ es-module-shims.js:475
(anonymous) @ es-module-shims.js:2

zachsa avatar Jun 24 '23 20:06 zachsa

Here's a reference to some code where it's easy to reproduce the race condition: https://github.com/zachsa/es-module-shims-jsx-plugin/tree/7574a1645874bb43fd37e6a5dbd1e24a7f603623

zachsa avatar Jun 25 '23 06:06 zachsa

I cloned the example and ran it in latest Chrome, but didn't get any race condition. What browser are you using?

If you want to avoid the plugin parsing all sources, you can use the skip option - https://github.com/guybedford/es-module-shims#skip. I'm not sure if it works with shim mode, but it might?

guybedford avatar Jun 26 '23 02:06 guybedford

Sorry - you have to move the dist import to below the es-module-shim import and then refresh the page a number of times

zachsa avatar Jun 26 '23 03:06 zachsa

If you move the dist import below the es module shim import then the plugin will be loaded after es-module-shims, therefore before the plugin loads es-module-shims will try to load without jsx support. So that would explain your race condition? So don't move the dist import below es module shims?

guybedford avatar Jun 26 '23 03:06 guybedford

Thank you - the documentation is becoming clearer now generally. Skip should be good in this case.

I'll create a branch with the race condition

zachsa avatar Jun 26 '23 03:06 zachsa

Would it not always fail then? That wasn't the case

zachsa avatar Jun 26 '23 03:06 zachsa

I guess it passes if the fetch request takes longer to load the first JSX file than it does to load the plugin?

guybedford avatar Jun 26 '23 07:06 guybedford

That is probably the issue

zachsa avatar Jun 26 '23 08:06 zachsa

f you want to avoid the plugin parsing all sources, you can use the skip option - https://github.com/guybedford/es-module-shims#skip. I'm not sure if it works with shim mode, but it might?

It does not appear to work in shim mode, or at least I get the error (using the debug build)

# In the code
globalThis.esmsInitOptions.shimMode = true
globalThis.esmsInitOptions.skip = /http:\/\/*/

# The errors
es-module-shims.debug.js:836 Uncaught TypeError: skip is not a function
    at es-module-shims.debug.js:836:21
    at async Promise.all (:3000/test/index 0)
    at async es-module-shims.debug.js:828:17

However, if I define skip as a string then that error disappears and another once appears instead:

globalThis.esmsInitOptions.skip = "https://*

Uncaught TypeError: Failed to resolve module specifier "@mui/system". Relative references must start with either "/", "./", or "../".

Is this something that I can request? Alternatively, do you have any suggestions for work arounds?

zachsa avatar Jun 27 '23 19:06 zachsa

Ahh right it seems skip doesn't work for shim mode - in that case you would want to rather create your own "skip" hook, by simply not doing the transformation by filtering it by URL in your own plugin hook. It's one if statement!

guybedford avatar Jun 30 '23 20:06 guybedford

I did try that... Will look into it before saying it didn't work

zachsa avatar Jul 01 '23 05:07 zachsa

I want to combine native importmaps and shim importmaps, native ones does not support extending import map in runtime. Shim mode affects the performance. So I want to use react, react-dom and some other dependencies for my main application, and for module-shim scripts also.

    <script>
        window.esmsInitOptions = {
            shimMode: true,
            mapOverrides: true,
            resolve: function (id, parentUrl, defaultResolve) {
                if (id === "react" || id === "reactDOM") {
                    // Some logic to prevent shim resolvers and use native imports???
                }
                return defaultResolve(id, parentUrl);
            }
        }
    </script>
    <script id="importmap" type="importmap">
        {
            "imports": {
                "react": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react.production.min.js",
                "reactDOM": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react-dom.production.min.js"
            }
        }
    </script>
    <script type="module" src="..."></script>

Then in runtime

  importShim.addImportMap({
    imports: importMap
  });
  
  const script = createHtmlElement("script", {
    type: "module-shim",
    src: component.url,
  });

How I can combine them? Dublicating react to shim importmap will broke react, it will throw this error.

mix0000 avatar Aug 14 '23 16:08 mix0000

You're exactly looking for the skip option here @mix0000 - this provides a filter for modules that should just directly defer to the native loader.

guybedford avatar Aug 15 '23 04:08 guybedford

@guybedford What I'm doing wrong?

    <script async src="https://ga.jspm.io/npm:[email protected]/dist/es-module-shims.js"></script>
    <script>
        window.esmsInitOptions = {
            shimMode: true,
            mapOverrides: true,
            skip: ["react", "reactDOM"]
        }
    </script>
    <script type="importmap">
        {
            "imports": {
                "react": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react.production.min.js",
                "reactDOM": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react-dom.production.min.js"
            }
        }
    </script>
    // This module is working
    <script type="module" src="..."></script>

Then in code

  importShim.addImportMap({
    imports: importMap
  });
  
  const script = createHtmlElement("script", {
    type: "module-shim",
    src: component.url
  });

And then I get this error.

image

I dont know why but in the code you firstly trying to resolve import and only then checks if its skipped.

image

mix0000 avatar Aug 16 '23 07:08 mix0000

Here is the solution for combining native importmap imports and shim imports:

For native importmaps:

<script type="importmap">
    {
        "imports": {
            "react": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react.production.min.js",
            "reactDOM": "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react-dom.production.min.js"
        }
    }
</script>
<!-- Load script using native importmaps so performance will not affected -->
<script type="module" src="..."></script>

And combining native importmaps with importmap shims Dynamically loaded scripts will use react from native importmaps and other dependencies will loaded by importmap shim.

const ignoreMap = {
    react: "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react.production.min.js",
    reactDOM: "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react-dom.production.min.js"
}
window.esmsInitOptions = {
    shimMode: true,
    mapOverrides: true,
    skip: Object.values(ignoreMap),
    resolve: function (id, parentUrl, defaultResolve) {
        if (id in ignoreMap) {
            return ignoreMap[id];
        }
        return defaultResolve(id, parentUrl);
    }
}

mix0000 avatar Aug 16 '23 09:08 mix0000

@mix0000 thanks very much for showing me how to override the resolve function. I'm still struggling to get this to work.

@guybedford previously showed me how to intercept fetch requests to modules:

async function compile(url, source) {
  const transformed = transform(source, {
    presets: [presetReact],
  })
  return transformed.code
}

globalThis.esmsInitOptions.fetch = async function (url, options) {
  const res = await fetch(url, options)
  if (!res.ok) return res
  const source = await res.text()
  const transformed = await compile(url, source)
  return new Response(new Blob([transformed], { type: 'application/javascript' }))
}

I was surprised to see that I couldn't simply return the original source code in the compile function depending on the URL.

For example, this doesn't work:

async function compile(url, source) {
  if (url.includes('http://localhost')) {
    const transformed = transform(source, {
      presets: [presetReact],
    })
    return transformed.code
  }
  return source
}

Is this working correctly @guybedford? From what I can tell, either I'm returning transformed.code, which is source code as text or I'm returning source, which is source code as text. I'm not sure why returning source would result in an error:

This code:

async function compile(url, source) {
  if (url.includes('http://localhost')) {
    const transformed = transform(source, {
      presets: [presetReact],
    })
    return transformed.code
  }
  return source
}

Gives me the error:

Uncaught TypeError: Failed to construct 'URL': Invalid base URL
    at pushSourceURL (es-module-shims.debug.js:676:25)
    at resolveDeps (es-module-shims.debug.js:693:7)
    at resolveDeps (es-module-shims.debug.js:597:7)
    at resolveDeps (es-module-shims.debug.js:597:7)
    at topLevelLoad (es-module-shims.debug.js:548:5)
pushSourceURL @ es-module-shims.debug.js:676
resolveDeps @ es-module-shims.debug.js:693
resolveDeps @ es-module-shims.debug.js:597
resolveDeps @ es-module-shims.debug.js:597
topLevelLoad @ es-module-shims.debug.js:548
Promise.catch (async)
processScript @ es-module-shims.debug.js:960
processScriptsAndPreloads @ es-module-shims.debug.js:872
(anonymous) @ es-module-shims.debug.js:506
Promise.then (async)
(anonymous) @ es-module-shims.debug.js:481
(anonymous) @ es-module-shims.debug.js:976
// In the 'resolveDeps' function in es-module-shims
    function pushSourceURL (commentPrefix, commentStart) {
      const urlStart = commentStart + commentPrefix.length;
      const commentEnd = source.indexOf('\n', urlStart);
      const urlEnd = commentEnd !== -1 ? commentEnd : source.length;
      pushStringTo(urlStart);
      resolvedSource += new URL(source.slice(urlStart, urlEnd), load.r).href;
      lastIndex = urlEnd;
    }

But the compile function like this doesn't error:

async function compile(url, source) {
  if (url === 'https://ga.jspm.io/npm:@mui/[email protected]/index.js') {
    console.log('im called') // this does log
    return source
  }
  const transformed = transform(source, {
    presets: [presetReact],
  })
  return transformed.code
}

I can see that when "//# sourceMappingURL=index.js.map" is included in the source, that I get the parse error above. This works:

async function compile(url, source) {
  if (url.includes('http://localhost')) {
    const transformed = transform(source, {
      presets: [presetReact],
    })
    return transformed.code
  }
  return source.replace(/\/\/#.*/, '')
}

Why would the presence of a source map cause es-module-shims to error?

Also, regarding the resolve override above - I'm not sure how that would work alongside overriding globalThis.fetch. Do you have any pointers?

zachsa avatar Sep 04 '23 12:09 zachsa

At a guess... it looks like this code:

resolvedSource += new URL(source.slice(urlStart, urlEnd), load.r).href;

is expecting a source map as a valid URL - but instead it's getting index.js.map, which it can't do anything with. I guess it's because es-module-shims can't resolve the source map, and babel.transform is stripping the source map reference

zachsa avatar Sep 04 '23 12:09 zachsa

@zachsa this error - Uncaught TypeError: Failed to construct 'URL': Invalid base URL means that the mistake already happened for the parent module, not the resolutions happening in the current module. Basically you have a load.u or load.r (u means URL, r means final redirected URL in the naming convention) that is not a valid URL so the new URL() is failing on the load.r value not being a URL, which was set already when the parent module was loaded. If you're using non standard URLs or paths for your module resolver hook that's going to be a bad time.

guybedford avatar Sep 05 '23 05:09 guybedford

@guybedford - I'm not sure I understand.

Here is one of the imports in my import map:

  {
    "imports": {
      "@emotion/react": "https://ga.jspm.io/npm:@emotion/[email protected]/dist/emotion-react.browser.esm.js",
      ...

and that JavaScript source file from ga.jspm.io includes a source map url

... some JavaScript

//# sourceMappingURL=emotion-react.browser.esm.js.map

With the following esmInitOptions:

globalThis.esmsInitOptions = {
  ...(globalThis.esmsInitOptions || {}),
  shimMode: true,
  async fetch(url, options) {
    const res = await fetch(url, options)
    if (!res.ok) return res
    const source = await res.text()
    const transformed = await compile(url, source)
    return new Response(new Blob([transformed], { type: 'application/javascript' }))
  },
}

Implementing the compile function:

This works

async function compile(url, source) {
  if (url.includes('http://localhost')) {
    const transformed = transform(source, {
      presets: [presetReact],
    })
    return transformed.code
  }
  return source.replace(/\/\/#.*/, '')
}

But this does not

async function compile(url, source) {
  if (url.includes('http://localhost')) {
    const transformed = transform(source, {
      presets: [presetReact],
    })
    return transformed.code
  }
  return source // <- This is different
}

The only difference is that in the working compile implementation I'm removing the source map reference at the bottom of the incoming source code.

so in the es-module-shim code:

function pushSourceURL (commentPrefix, commentStart) {
  const urlStart = commentStart + commentPrefix.length;
  const commentEnd = source.indexOf('\n', urlStart);
  const urlEnd = commentEnd !== -1 ? commentEnd : source.length;
  pushStringTo(urlStart);
  console.log(load.u) // prints https://ga.jspm.io/npm:[email protected]/index.js
  console.log(urlStart, urlEnd, source.slice(urlStart, urlEnd)) // This prints the source map URL as it comes from the JSPM CDN => 7765 7777 index.js.map
  resolvedSource += new URL(source.slice(urlStart, urlEnd), load.r).href;
  lastIndex = urlEnd;
}

So in my code, why would the load.r value be different in my case compared to when using es-module-shims in the normal way.

It doesn't seem to matter whether I'm in shimMode = true or false as things seems to function the same way - i.e. I guess that overriding the fetch function negates the difference between shimMode true / false?

zachsa avatar Sep 12 '23 13:09 zachsa

Actually.. Thinking of this. With shimMode false I would not expect the type="module-shim" script to be executed at all, but it is.

zachsa avatar Sep 12 '23 19:09 zachsa

Any single module-shim script will enable shim mode automatically.

@zachsa I believe the point remains that your resolve hook is incorrect, even if the bug only triggers when there is a source map.

I would suggest logging the value of load.r here to determine why it has a bad base for resolution.

guybedford avatar Sep 13 '23 05:09 guybedford

load.r is a blank string load.r === '' // true

However I can fix the issue by changing the fetch hook:

async fetch(url, options) {
  const res = await fetch(url, options)
  if (!res.ok) return res
  const source = await res.text()
  const transformed = url.includes('http://localhost') ? await compile(url, source) : source
  return new Response(new Blob([transformed], { type: 'application/javascript' }))
}

Looking at the docs I see that I should return the res, not the res text if I want to escape this hook. This works:

async fetch(url, options) {
  const res = await fetch(url, options)
  if (!res.ok) return res
  if (url.includes('http://localhost')) {
    const source = await res.text()
    const transformed = await compile(url, source)
    return new Response(new Blob([transformed], { type: 'application/javascript' }))
  }
  return res
}

zachsa avatar Sep 13 '23 06:09 zachsa

Thanks for your patience.

Last question - what would the benefit of a custom resolve hook be since I'm already including a fetch hook to transform code from some URLs (i.e localhost)?

Specifically I'm wondering if there are possibilities that I haven't thought of. In @mix0000's code above:

const ignoreMap = {
    react: "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react.production.min.js",
    reactDOM: "https://cdn.jsdelivr.net/npm/@esm-bundle/[email protected]/esm/react-dom.production.min.js"
}
window.esmsInitOptions = {
    shimMode: true,
    mapOverrides: true,
    skip: Object.values(ignoreMap),
    resolve: function (id, parentUrl, defaultResolve) {
        if (id in ignoreMap) {
            return ignoreMap[id];
        }
        return defaultResolve(id, parentUrl);
    }
}

Would including a resolve hook / skip of all imports present in an import map effect my example in any way?

zachsa avatar Sep 13 '23 07:09 zachsa