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

UMD/CJS formats: handle (top-level) await import(...) as require(...) #3621

Open
michael-brade opened this issue Jun 7, 2020 · 7 comments
Open

Comments

@michael-brade
Copy link

michael-brade commented Jun 7, 2020

Feature Use Case

I have code that runs in the browser and on server-side node. On the server-side, I need to import a DOM implementation, in the browser obviously not. I want to export that code as UMD and ES.

So I have written this, which would work:

if (typeof window === 'undefined') {
  try {
    createHTMLWindow = require('svgdom').createHTMLWindow;
  } catch (e) {
    svgdom = await import('svgdom');
    createHTMLWindow = svgdom.createHTMLWindow;
  }
  global.window = createHTMLWindow();
  global.document = window.document;
}

If we are in the browser, this code does nothing. But in node, it tries to require it in case this wasn't loaded as a module and top-level awaits if this was a module.

However, running this through rollup won't work with any configuration I tried.

Feature Proposal

UPDATE: see the next comment for an updated proposal

So what I need is for rollup to ignore the await import('svgdom'); if in rollup.config.js there is the external: ['svgdom'] option for formats other than ES and System. Otherwise, it complains that TLA is not supported for UMD.

Another solution might be to flag sections of code that should not be changed and literally rendered by rollup. Or to simply ignore sections of code conditionally.

If you have any other suggestions or solution, I would of course be happy to try them.

@michael-brade
Copy link
Author

michael-brade commented Jun 7, 2020

Actually, I think I have a better proposal:

Why not handle a (top-level) await import(...) as if it was a require(...) if the format is UMD or CJS?

@lukastaegert
Copy link
Member

Like this? https://rollupjs.org/repl/?version=2.14.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCgnLiUyRmZvby5qcycpLnRoZW4oZm9vJTIwJTNEJTNFJTIwY29uc29sZS5sb2coZm9vKSklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJmb28uanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwZGVmYXVsdCUyMDQyJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmNqcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

Of course you cannot do this for IIFE or in extension UMD because there is no global variable require in a script tag IIFE environment—the code would just throw. In general, dynamic imports are just not supported for those formats in Rollup because there is no generic way to make them work. Top level await is an even bigger problem. At the moment, we do not have a way to make this work for formats other than ESM or SystemJS because for other formats, it is impossible to make it work without huge transformations of the code. You might have some luck with something akin to https://github.com/eponymous-labs/babel-plugin-top-level-await but I would guess it is more pain than it is worth.

My question is: Is it really important to do environment auto-detection on runtime, or would it work for you to just have a bundle with the dependency and another without the dependency? To easily split the two, you could put the code above into a separate module that is imported by your entry point. Then for the browser bundle, use something like https://github.com/proteriax/rollup-plugin-ignore to turn the module into something without side-effects that will be removed by tree-shaking. This will be a little more difficult for the user to handle but will produce much better bundles as there is no danger a user's browser build accidentally bundles svgdom.

@michael-brade
Copy link
Author

Hmm, I am not sure what that REPL example is supposed to tell me :-D

And yes, I know that TLA in general is nearly impossible to get to work for umd/cjs. However, the special case of

... = await import(...)

should be completely equivalent to

... = require(...)

in case of umd/cjs, no?

I agree with your rest, though 😢 I tried to find even one hack that would work - nothing does, not that way anyway. I would have to rewrite my code, too, so now I gave up and just assume that any node user will have to provide a DOM implementation before importing my lib. Until the future fixes all those problems ;-)

[I didn't choose the split node/browser solution because that would double the size of the package, and because I didn't know about rollup-plugin-ignore when I decided it :p]

@michael-brade
Copy link
Author

PS: I may not have put it clearly enought: when I said "completely equivalent" I didn't mean that that should be the output of rollup but that the meaning is equivalent. I.e., for UMD/CJS that particular TLA should be handled by rollup like if it was a ... = require(...).

@michael-brade michael-brade changed the title Allow rollup to ignore code, or: ignore top-level await on imports that are external UMD/CJS formats: handle (top-level) await import(...) as require(...) Jun 8, 2020
@jimmywarting
Copy link

I stumble upon some similar issue...

i have conditional import... looks something like this:

if (!globalThis.ReadableStream) {
  try {
    Object.assign(globalThis, require('stream/web'))
  } catch (error) {
    Object.assign(globalThis, require('web-streams-polyfill/dist/ponyfill.es2018.js'))
  }
}

what the actual output is:

import require$$0 from 'stream/web';
// ...

if (!globalThis.ReadableStream) {
  try {
    Object.assign(globalThis, require$$0);
  } catch (error) {
    Object.assign(globalThis, ponyfill_es2018.exports);
  }
}

It would have been nice if it could do top-level await instead... cuz right now somebody gets a
Module not found: Error: Can't resolve 'stream/web' node-fetch/fetch-blob#117


...and this two packages that i own:

...they import some stuff i tried to avoid importing i don't want to bring in node:buffer or node:os stuff to the web

if i use something like a cdn like jsDeliver https://cdn.jsdelivr.net/npm/stream-consumers@0.1.0/mod.js
then i get exactly what i want

I guess some of it could have been solved with top level await...

@benmccann
Copy link
Contributor

I stumble upon some similar issue...

I didn't look at the original issue reported here, but the issue from this last post is actually coming from @rollup/plugin-commonjs: rollup/plugins#1004

@jimmywarting
Copy link

sooo... rollup/plugins#1005 is merged, maybe this can be closed now?
not involved in rollup or anything maybe just need to update one dependency on rollup/plugin or something...

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

4 participants