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

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

Open
zachsa opened this issue Jun 24, 2023 · 24 comments

Comments

@zachsa
Copy link

zachsa commented Jun 24, 2023

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 <script type="importmap">`. For example,

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 zachsa changed the title Scripts of type "module-shim" don't seem to have access to the import map Scripts of type "module-shim" don't seem to have access to the script of type="importmap" defined in the HTML head Jun 24, 2023
@zachsa zachsa changed the title Scripts of type "module-shim" don't seem to have access to the script of type="importmap" defined in the HTML head Scripts of type "module-shim" don't seem to have access to specifier defined in importmap Jun 24, 2023
@zachsa
Copy link
Author

zachsa commented Jun 24, 2023

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
Copy link
Author

zachsa commented Jun 25, 2023

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

@guybedford
Copy link
Owner

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?

@zachsa
Copy link
Author

zachsa commented Jun 26, 2023

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

@guybedford
Copy link
Owner

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?

@zachsa
Copy link
Author

zachsa commented Jun 26, 2023

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
Copy link
Author

zachsa commented Jun 26, 2023

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

@guybedford
Copy link
Owner

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

@zachsa
Copy link
Author

zachsa commented Jun 26, 2023

That is probably the issue

@zachsa
Copy link
Author

zachsa commented Jun 27, 2023

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?

@guybedford
Copy link
Owner

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!

@zachsa
Copy link
Author

zachsa commented Jul 1, 2023

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

@mix0000
Copy link

mix0000 commented Aug 14, 2023

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.

@guybedford
Copy link
Owner

guybedford commented Aug 15, 2023

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.

@mix0000
Copy link

mix0000 commented Aug 16, 2023

@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
Copy link

mix0000 commented Aug 16, 2023

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);
    }
}

@zachsa
Copy link
Author

zachsa commented Sep 4, 2023

@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
Copy link
Author

zachsa commented Sep 4, 2023

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

@guybedford
Copy link
Owner

@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.

@zachsa
Copy link
Author

zachsa commented Sep 12, 2023

@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
Copy link
Author

zachsa commented Sep 12, 2023

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

@guybedford
Copy link
Owner

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.

@zachsa
Copy link
Author

zachsa commented Sep 13, 2023

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
Copy link
Author

zachsa commented Sep 13, 2023

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?

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