-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
Allow ignoring dynamic imports #1184
Comments
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. |
Fair enough, I think Rollup maintainers agree with you which is why they made it a warning instead of a fatal error.
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:
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 |
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 |
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 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.
No I'm not targeting node, targeting browser. |
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 |
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 |
Here's the repro for the issue: Bundling will error with: Stacktrace
Bundling using |
Yep, as I thought. It just fails at the next unsupported thing when I remove the |
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:
|
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:
The Closure Compiler does have an option
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
|
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. |
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. |
#1084 (comment) 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:
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:
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 #1084 (comment)
The text was updated successfully, but these errors were encountered: