search

thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.23k stars 173 forks source link

Allow ignoring dynamic imports #1184

Open jorenbroekema opened 4 weeks ago

jorenbroekema commented 4 weeks ago

https://github.com/thheller/shadow-cljs/issues/1084#issuecomment-2152458452 original comment but that issue is closed.

I know dynamic imports aren't supported, I won't ask for this but what I would like to ask is some kind of feature to ignore them or prevent errors from being thrown, similar to how this is possible in bundlers like Rollup, Webpack, Vite, etc.

Let's take the use case where certain libraries will dynamically import from resources such as a CDN.

For example:

const CDN_URL = `https://ga.jspm.io/npm:${package}@${version}/${entrypoint}`;

const mod = await import(CDN_URL);

Bundlers in this case shouldn't even attempt to bundle the dynamic import, it doesn't need to analyze it and in the above example the CDN_URL variable isn't statically analyzable to begin with because it's a variable rather than a raw string.

Rollup will just ignore bundling such a dynamic import and give the user a warning, which in this case is desirable. Webpack and Vite will error unless you wrap the import parameter in an ignore statement:

const CDN_URL = `https://ga.jspm.io/npm:${package}@${version}/${entrypoint}`;

const mod = await import(/* @vite-ignore */ /* webpackIgnore: true */ CDN_URL);

I think it would be cool if shadow-cljs could add a way to ignore certain dynamic imports similar to Vite or Webpack, or at least some way to prevent it from throwing an error (e.g. in Rollup you can turn the warning off as well).

Even better of course if this bundler supports dynamic imports but until then this would already be a quick win for dynamic imports that are not intended to be analyzed by bundlers.

Originally posted by @jorenbroekema in https://github.com/thheller/shadow-cljs/issues/1084#issuecomment-2152458452

thheller commented 4 weeks ago

Do you have a concrete example of a library on npm doing this?

I'm not against shadow-cljs supporting this. It is more an issue with the Closure Compiler itself. It does not like dynamic import, so shadow-cljs is already working around several cases for npm packages.

import(/* @vite-ignore */ /* webpackIgnore: true */ CDN_URL);

This is the perfect example of why the JS world is such a mess though. I certainly won't be supporting this build-tool specific comment nonsense.

jorenbroekema commented 4 weeks ago

This is the perfect example of why the JS world is such a mess though. I certainly won't be supporting this build-tool specific comment nonsense.

Fair enough, I think Rollup maintainers agree with you which is why they made it a warning instead of a fatal error.

Do you have a concrete example of a library on npm doing this?

Any library that allows you to write your config in javascript files and expect a default export of that config object, it then imports those user configs as JS modules using dynamic imports.

Biggest examples from the top of my head:

Small disclaimer: I maintain Style Dictionary and I am the reason for the dynamic imports there, that said, there isn't a way around it, just used to be CommonJS syntax in the past as opposed to ESM nowadays.

It's surprisingly common in JS ecosystem 😅

One of the reasons why perhaps this hasn't been much of an issue in the past is because those tools were all NodeJS-only tools using CommonJS syntax which is using require() to load other modules, and these are synchronous and there's no distinction between "dynamic" and "static" like there is for ES Module imports

thheller commented 4 weeks ago

None of those are really relevant examples meant to run in the browser where it would be relevant for shadow-cljs to process them? Are you targetting node? What build :target do you use?

jorenbroekema commented 4 weeks ago

Prettier, Rollup and Style Dictionary all can run in the browser, and with File Access API being used more and more I expect these examples to start showing up even more, because then dynamic imports could be done from a browser environment to a user's filesystem. There are some really interesting NPM packages that create node:fs browser shims that work in memory, and they have adapters for this file system API to make it work with the actual filesystem of the user. https://github.com/streamich/memfs/tree/master/demo/fsa-to-node-zipfile this is one such example, it has a couple of others in that repository as well. Even though those examples are using fs.readFile and not import(), when the file you want to load is a JS module and you want it to be interpreted as such, you'd be using import(). Unfortunately the examples I have for that are closed-source so I can't share more details about it.

Another major use case for dynamic imports are importing modules from CDNs, which is used a lot (but not exclusive to) in microfrontends, this is a pattern that's pretty common among large enterprise companies where they want to leverage microfrontends e.g. because:

I also can't share any of the CDN utilities used in those companies that I worked for because they're also closed-source.

Are you targetting node? What build :target do you use?

No I'm not targeting node, targeting browser.

jorenbroekema commented 4 weeks ago

Ah I just remembered another more concrete example: REPLs

One example is Rollup REPL https://rollupjs.org/repl/ which also uses dynamic imports under the hood to be able to load the JS files you write in the playground which it will then bundle. This uses @rollup/browser adapter.

Another example is Style Dictionary Configurator https://configurator.tokens.studio/ which also allows you to write JS files in-browser which is dynamically loads under the hood, another example is the spin-off REPL on the main site here https://v4.styledictionary.com/ -> live demo. The default example uses JSON files but it works with JS files (default export) as well, and uses import() for those

thheller commented 4 weeks ago

I'm going to need a example repro showing what you are trying to do and how shadow-cljs fails to make it work.

I frankly have very little hope of shadow-cljs being able to bundle packages that inherently target node. It does lack many node-isms that other JS bundlers support and I have no interest in supporting them.

shadow-cljs does support dynamic import in CLJS code, see this post. It just doesn't for npm packages given that many packages actually use dynamic import and expect the bundler to bundle it.

You might have more luck overall using :target :esm + :js-provider :import or :target :npm-module and post-processing the output with an actual JS build tool.

floscr commented 3 weeks ago

Here's the repro for the issue: https://github.com/tokens-studio/style-dictionary-shadow-cljs

Bundling will error with:

Stacktrace ``` file uses import() with unsupported arguments and cannot be processed Node(DYNAMIC_IMPORT): /home/floscr/Code/Repositories/shadow-cljs-quickstart/node_modules/style-dictionary/lib/StyleDictionary.js:220:25 [source unknown] Parent(AWAIT): /home/floscr/Code/Repositories/shadow-cljs-quickstart/node_modules/style-dictionary/lib/StyleDictionary.js:220:19 [source unknown] com.google.javascript.jscomp.Compiler.throwInternalError (Compiler.java:3282) com.google.javascript.jscomp.NodeTraversal.throwUnexpectedException (NodeTraversal.java:516) com.google.javascript.jscomp.NodeTraversal.traverse (NodeTraversal.java:536) com.google.javascript.jscomp.NodeTraversal$Builder.traverse (NodeTraversal.java:472) com.google.javascript.jscomp.NodeTraversal.traverse (NodeTraversal.java:542) shadow.build.closure.JsInspector.getFileInfo (JsInspector.java:205) shadow.build.closure.JsInspector.getFileInfoMap (JsInspector.java:211) shadow.build.npm/get-file-info*/fn--10814 (npm.clj:431) shadow.build.npm/get-file-info* (npm.clj:430) shadow.build.npm/get-file-info* (npm.clj:380) shadow.build.npm/get-file-info (npm.clj:491) shadow.build.npm/get-file-info (npm.clj:488) shadow.build.npm/file-as-resource (npm.clj:665) shadow.build.npm/file-as-resource (npm.clj:663) shadow.build.npm/find-resource-from-exports-exact (npm.clj:738) shadow.build.npm/find-resource-from-exports-exact (npm.clj:715) shadow.build.npm/find-resource-from-exports (npm.clj:807) shadow.build.npm/find-resource-from-exports (npm.clj:804) shadow.build.npm/find-resource-in-package (npm.clj:828) shadow.build.npm/find-resource-in-package (npm.clj:813) shadow.build.npm/find-resource (npm.clj:949) shadow.build.npm/find-resource (npm.clj:876) shadow.build.resolve/find-npm-resource (resolve.clj:123) shadow.build.resolve/find-npm-resource (resolve.clj:94) shadow.build.resolve/fn--11439 (resolve.clj:266) shadow.build.resolve/fn--11439 (resolve.clj:233) clojure.lang.MultiFn.invoke (MultiFn.java:244) shadow.build.resolve/find-resource-for-string (resolve.clj:81) shadow.build.resolve/find-resource-for-string (resolve.clj:70) shadow.build.resolve/resolve-string-require (resolve.clj:464) shadow.build.resolve/resolve-string-require (resolve.clj:447) shadow.build.resolve/resolve-require (resolve.clj:700) shadow.build.resolve/resolve-require (resolve.clj:693) shadow.build.resolve/resolve-deps/fn--11390 (resolve.clj:52) clojure.lang.PersistentVector.reduce (PersistentVector.java:343) clojure.core/reduce (core.clj:6885) clojure.core/reduce (core.clj:6868) shadow.cljs.util/reduce-> (util.clj:42) shadow.cljs.util/reduce-> (util.clj:41) shadow.build.resolve/resolve-deps (resolve.clj:50) shadow.build.resolve/resolve-deps (resolve.clj:34) shadow.build.resolve/resolve-symbol-require (resolve.clj:687) shadow.build.resolve/resolve-symbol-require (resolve.clj:647) shadow.build.resolve/resolve-require (resolve.clj:697) shadow.build.resolve/resolve-require (resolve.clj:693) shadow.build.resolve/resolve-entry (resolve.clj:707) shadow.build.resolve/resolve-entry (resolve.clj:706) clojure.lang.PersistentVector.reduce (PersistentVector.java:343) clojure.core/reduce (core.clj:6885) clojure.core/reduce (core.clj:6868) shadow.cljs.util/reduce-> (util.clj:42) shadow.cljs.util/reduce-> (util.clj:41) shadow.build.resolve/resolve-entries (resolve.clj:721) shadow.build.resolve/resolve-entries (resolve.clj:712) shadow.build.modules/resolve-module/fn--14042 (modules.clj:252) shadow.build.modules/resolve-module (modules.clj:248) shadow.build.modules/resolve-module (modules.clj:238) clojure.lang.PersistentVector.reduce (PersistentVector.java:343) clojure.core/reduce (core.clj:6885) clojure.core/reduce (core.clj:6868) shadow.build.modules/resolve-modules (modules.clj:258) shadow.build.modules/resolve-modules (modules.clj:257) shadow.build.modules/analyze (modules.clj:312) shadow.build.modules/analyze (modules.clj:303) shadow.build/resolve (build.clj:459) shadow.build/resolve (build.clj:453) shadow.build/compile (build.clj:509) shadow.build/compile (build.clj:493) shadow.cljs.devtools.server.worker.impl/build-compile (impl.clj:368) shadow.cljs.devtools.server.worker.impl/build-compile (impl.clj:349) shadow.cljs.devtools.server.worker.impl/fn--15664 (impl.clj:448) shadow.cljs.devtools.server.worker.impl/fn--15664 (impl.clj:437) clojure.lang.MultiFn.invoke (MultiFn.java:234) shadow.cljs.devtools.server.util/server-thread/fn--15432/fn--15433/fn--15441 (util.clj:283) shadow.cljs.devtools.server.util/server-thread/fn--15432/fn--15433 (util.clj:282) shadow.cljs.devtools.server.util/server-thread/fn--15432 (util.clj:255) java.lang.Thread.run (Thread.java:840) Caused by: IllegalArgumentException: file uses import() with unsupported arguments and cannot be processed shadow.build.closure.JsInspector$FileInfo.visit (JsInspector.java:132) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:961) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseFunction (NodeTraversal.java:1006) com.google.javascript.jscomp.NodeTraversal.handleFunction (NodeTraversal.java:857) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:903) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.handleClassMembers (NodeTraversal.java:1108) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:912) com.google.javascript.jscomp.NodeTraversal.handleClass (NodeTraversal.java:1050) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:909) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:951) com.google.javascript.jscomp.NodeTraversal.traverseChildren (NodeTraversal.java:1134) com.google.javascript.jscomp.NodeTraversal.handleModule (NodeTraversal.java:871) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:906) com.google.javascript.jscomp.NodeTraversal.traverseChildren (NodeTraversal.java:1134) com.google.javascript.jscomp.NodeTraversal.handleScript (NodeTraversal.java:845) com.google.javascript.jscomp.NodeTraversal.traverseBranch (NodeTraversal.java:900) com.google.javascript.jscomp.NodeTraversal.traverse (NodeTraversal.java:533) com.google.javascript.jscomp.NodeTraversal$Builder.traverse (NodeTraversal.java:472) com.google.javascript.jscomp.NodeTraversal.traverse (NodeTraversal.java:542) shadow.build.closure.JsInspector.getFileInfo (JsInspector.java:205) shadow.build.closure.JsInspector.getFileInfoMap (JsInspector.java:211) shadow.build.npm/get-file-info*/fn--10814 (npm.clj:431) shadow.build.npm/get-file-info* (npm.clj:430) shadow.build.npm/get-file-info* (npm.clj:380) shadow.build.npm/get-file-info (npm.clj:491) shadow.build.npm/get-file-info (npm.clj:488) shadow.build.npm/file-as-resource (npm.clj:665) shadow.build.npm/file-as-resource (npm.clj:663) shadow.build.npm/find-resource-from-exports-exact (npm.clj:738) shadow.build.npm/find-resource-from-exports-exact (npm.clj:715) shadow.build.npm/find-resource-from-exports (npm.clj:807) shadow.build.npm/find-resource-from-exports (npm.clj:804) shadow.build.npm/find-resource-in-package (npm.clj:828) shadow.build.npm/find-resource-in-package (npm.clj:813) shadow.build.npm/find-resource (npm.clj:949) shadow.build.npm/find-resource (npm.clj:876) shadow.build.resolve/find-npm-resource (resolve.clj:123) shadow.build.resolve/find-npm-resource (resolve.clj:94) shadow.build.resolve/fn--11439 (resolve.clj:266) shadow.build.resolve/fn--11439 (resolve.clj:233) clojure.lang.MultiFn.invoke (MultiFn.java:244) shadow.build.resolve/find-resource-for-string (resolve.clj:81) shadow.build.resolve/find-resource-for-string (resolve.clj:70) shadow.build.resolve/resolve-string-require (resolve.clj:464) shadow.build.resolve/resolve-string-require (resolve.clj:447) shadow.build.resolve/resolve-require (resolve.clj:700) shadow.build.resolve/resolve-require (resolve.clj:693) shadow.build.resolve/resolve-deps/fn--11390 (resolve.clj:52) clojure.lang.PersistentVector.reduce (PersistentVector.java:343) clojure.core/reduce (core.clj:6885) clojure.core/reduce (core.clj:6868) shadow.cljs.util/reduce-> (util.clj:42) shadow.cljs.util/reduce-> (util.clj:41) shadow.build.resolve/resolve-deps (resolve.clj:50) shadow.build.resolve/resolve-deps (resolve.clj:34) shadow.build.resolve/resolve-symbol-require (resolve.clj:687) shadow.build.resolve/resolve-symbol-require (resolve.clj:647) shadow.build.resolve/resolve-require (resolve.clj:697) shadow.build.resolve/resolve-require (resolve.clj:693) shadow.build.resolve/resolve-entry (resolve.clj:707) shadow.build.resolve/resolve-entry (resolve.clj:706) clojure.lang.PersistentVector.reduce (PersistentVector.java:343) clojure.core/reduce (core.clj:6885) clojure.core/reduce (core.clj:6868) shadow.cljs.util/reduce-> (util.clj:42) shadow.cljs.util/reduce-> (util.clj:41) shadow.build.resolve/resolve-entries (resolve.clj:721) shadow.build.resolve/resolve-entries (resolve.clj:712) shadow.build.modules/resolve-module/fn--14042 (modules.clj:252) shadow.build.modules/resolve-module (modules.clj:248) ```

Bundling using :target :npm-module works, but as you said it would require post processing via another bundler.

thheller commented 3 weeks ago

Yep, as I thought. It just fails at the next unsupported thing when I remove the import() check. I see zero chance that shadow-cljs will be able to bundle this.

jorenbroekema commented 3 weeks ago

file uses import() with unsupported arguments and cannot be processed

I think the entire point I'm trying to make is that this should be a warning, the import() statement should be ignored, and it should not cause a fatal error, just continue as is.

This pattern: import(someVar) is not statically analyzable and there isn't any bundler out there that handles this, the only difference is that:

thheller commented 3 weeks ago

That error exists regardless of whether shadow-cljs throws it or not. The Closure Compiler is the bottleneck here. If I remove that check and just let it proceed as normal end fails at a later stage with:

Closure compilation failed with 1 errors
--- node_modules/dummy/index.js:6
Dynamic import expressions cannot be transpiled.

The Closure Compiler does have an option CompilerOptions.setAllowDynamicImport which defaults to false and leads to the above error. If I set that to true I get a bit further but ultimately end up with

IllegalStateException: AST should not contain Dynamic module import. Reference node:
DYNAMIC_IMPORT 6:9  [length: 11] [source_file: node_modules/dummy/index.js]
    NAME foo 6:16  [length: 3] [source_file: node_modules/dummy/index.js] [original_name: foo]

 Parent node:
RETURN 6:2  [length: 19] [source_file: node_modules/dummy/index.js]
    DYNAMIC_IMPORT 6:9  [length: 11] [source_file: node_modules/dummy/index.js]
        NAME foo 6:16  [length: 3] [source_file: node_modules/dummy/index.js] [original_name: foo]

    com.google.javascript.jscomp.AstValidator$1.handleViolation (AstValidator.java:88)
    com.google.javascript.jscomp.AstValidator.violation (AstValidator.java:2194)
    com.google.javascript.jscomp.AstValidator.validateFeature (AstValidator.java:2275)
    com.google.javascript.jscomp.AstValidator.validateExpression (AstValidator.java:515)
    com.google.javascript.jscomp.AstValidator.validateReturn (AstValidator.java:1628)
        ...

This is where I also stopped digging a year ago or whenever the last time was I tried to investigate this.

Potentially there is a fix but I couldn't tell you. If you want to dig feel free to do so.

FWIW this is the node_modules/dummy/index.js I tested this with

export default function(foo) {
   return import(foo);
};
jorenbroekema commented 3 weeks ago

I see, yeah I suppose the clojure compiler needs to compile it to something useful, it can't just ignore it? E.g. a web bundler can just go "okay fine we leave it as is and the browser env will handle it on-demand". I'm not sure if this principle can be applied to this context, my knowledge of Clojure and its compiler is simply way too limited to propose something useful there. Maybe @floscr has some suggestions.

@thheller thanks for digging into it with us btw, appreciated.

thheller commented 3 weeks ago

Please note its the Closure Compiler. Not Clojure. You probably mean that, just making sure to avoid confusion.

And yes, there are a lot of things I wish the Closure Compiler did differently, but it is not under my control and forking it is just not an option for me.