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

Skypack bundle cannot be loaded in the browser #489

Open
jgosmann opened this issue Jan 19, 2024 · 7 comments · May be fixed by #501
Open

Skypack bundle cannot be loaded in the browser #489

jgosmann opened this issue Jan 19, 2024 · 7 comments · May be fixed by #501

Comments

@jgosmann
Copy link

We understand you have a problem and are in a hurry, but please provide us with some info to make it much more likely for your issue to be understood, worked on and resolved quickly.

What did you expect to happen?
FakeTimers module is loaded.

What actually happens
An error is logged and the module does not load:

Uncaught Error: [Package Error] "timers" does not exist. (Imported by "@sinonjs/fake-timers").
    <anonymous> https://cdn.skypack.dev/error/node:timers?from=@sinonjs/fake-timers:14

How to reproduce

Go to https://output.jsbin.com/xijaquyico and open the JavaScript console to see the error. The module is loaded like this:

  <script type="module">
    import FakeTimers from "https://cdn.skypack.dev/@sinonjs/[email protected]";
  </script>
@jgosmann
Copy link
Author

With version 7.0.1 when the Skypack instructions were added to the readme, it still works.

I suppose the conversion to ESM destroys the conditional import here?

@fatso83
Copy link
Contributor

fatso83 commented Jan 19, 2024

oooh, good catch! We have not caught this, nor do we have a test for it. we could still do an async await import(...), though, to fix this, I guess, but that will be a breaking change, as that cannot be done without making the setup face async.

Or ... well.

It can be done, sort of, but it will be a bit hacky:

  • we can add an assumption that the timersModule import can either be undefined or a Promise. Let's just call it timerImport.
  • if we run withGlobal and timerImport is defined, we must require that the Promise is resolved before continuing
  • for environments where module is not defined, todays interface will continue as before, in others it will be breaking

Which is not great, as I assume most/many consumers will be using this in ESM, in which module will be defined (but in a lot of them, like browsers, there will of course not be a node:timers).

@mroderick I think you would be interested in this.

@SimenB Any ideas? Fixing conditional imports like this would trigger a breaking API change in Jest, right?

I am guessing someone has some clever ideas somewhere.

@benjamingr
Copy link
Member

@fatso83 nothing is stopping us from doing top-level await import right?

@fatso83
Copy link
Contributor

fatso83 commented Jan 19, 2024

No ... but I am not sure how that works. I usually try to map these newer constructs over to how this would look in ES5, but I cannot see how this works without introducing a Promise that leaks out into the exported module. But maybe there is? Totally not sure, so I will glady be informed 😅

@fatso83
Copy link
Contributor

fatso83 commented Feb 10, 2024

Gah, this was hard, Ben. I have tried out top-level await , but getting that and CJS to work together was non-trivial, so it ended up just being a ESM conversion. Did not get anywhere. I think this will have to wait until @mroderick does a full ESM conversion?

@fatso83 fatso83 linked a pull request Aug 26, 2024 that will close this issue
@fatso83
Copy link
Contributor

fatso83 commented Aug 26, 2024

Just to add some details on why this is not moving:

  • when using require the modules need to exist for Skypack to work
  • one can use import() in CJS, but one cannot use top level await outside of an async function:

/Users/carlerik/dev/fake-timers/src/fake-timers-src.js:20
await tryImports();
^^^^^

SyntaxError: await is only valid in async functions and the top level bodies of modules

So AFAIK, the only way of avoiding exposure of some Promise is moving the entire module to ESM, which I did here:
#501

This opens up a can of worms, as we should not do this without being prepared to convert Sinon as well.

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

It's not the only way, btw. We can re-introduce a bundler (which was removed in #345), which could simply create a bundle where those modules were not available, which is what is done in sinon and nise currently to have them bundle successfully: sinonjs/nise@1422ca7

I'd still much prefer having an ESM module, though that would force our hand to do the same for the main Sinon project.

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

Successfully merging a pull request may close this issue.

3 participants