writeConfig: Support `inlineDynamicImports: true`

👋🏽 Hi!

I'd like to consume a module – specifically @hotwired/turbo-rails – which dynamically imports another module (see https://github.com/hotwired/turbo-rails/blob/acbb9250cf97cbf9f00570cbfbdeeffc815003b8/app/javascript/turbo/cable.js#L11-L14).

rollup seems to automatically enable code splitting for the bundle, which causes faucet-pipeline-js to complain (see https://github.com/faucet-pipeline/faucet-pipeline-js/blob/b90282cd20308d57a40c5b49abf171847c82ad06/lib/bundle/bundler.js#L17-L19).

I could work around the issue by patching lib/bundle/bundler.js to inline the dynamic import:

diff --git a/lib/bundle/bundler.js b/lib/bundle/bundler.js
index f0fb301..3f2ffe1 100644
--- a/lib/bundle/bundler.js
+++ b/lib/bundle/bundler.js
@@ -11,6 +11,7 @@ module.exports = function generateBundle(entryPoint, config, cache) {
    return rollup.rollup(options).
        then(bundle => {
            cache = bundle;
+           writeConfig.inlineDynamicImports = true;
            return bundle.generate(writeConfig);
        }).
        then(({ output }) => {

… and now I got some questions and hope you can help me understanding the situation.

I understand that rollup does this automatic code splitting when detecting dynamic imports for some optimization (or whatever good reason they have :)).

I also understand that faucet-pipeline-js currently does not support a chunked result from rollup.

Given that these statements are correct, I understand that one could configure rollup to disable the dynamic import optimization but inlining the import, which would resolve my issue.

If this sounds reasonable: Would you be open for a PR that would allow passing such a configuration option directly to rollup?

Thanks for your thoughtful suggestion and for digging into the code for details!

TLDR: Yes, a PR would be most welcome.

I was a bit wary of this because it might unwittingly result in significant bloat:

switch(condition) {
case …: import(…);
case …: import(…);
case …: import(…);
case …: import(…);
}

However, @moonglum convinced me that it could be a generically useful feature, provided people know what they're doing - plus there's always the bundle-size warning if things get out of hand. So it has to be opt in, ideally prompted by a better error message than the above (something like "Error: We do not support multiple output files right now. See https://www.faucet-pipeline.org/faq#xyz for details." perhaps).

I'm unsure what that option might be called though: inlineDynamicImports is a bit clunky and, while reasonably descriptive, it might not be easy to decode if you don't already know what it's for. Plus grammatically it's a bit inconsistent with the current set of options, all of which we somehow managed to keep to a single word (more or less) - that's not a hard requirement, but it's worth pondering for a bit (FWIW, @moonglum already suggested dynamicImports: "inline", but wasn't entirely happy with that either).

PS: We do intend to support code splitting at some point, we just haven't quite had the headspace for it so far. Inlining would still be a useful option then, so this would be more than just a stopgap measure.

Wow. That is a super fast and thorough response!

I was a bit wary of this because it might unwittingly result in significant bloat:

Ah. I see.

… provided people know what they're doing - plus there's always the bundle-size warning if things get out of hand.

Yes. I agree that the effect could potentially be unexpectedtly huge.

… ideally prompted by a better error message than the above […] See faucet-pipeline.org/faq#xyz for details." perhaps).

Right. I could not find anything on https://www.faucet-pipeline.org/js regarding multiple output files. I assume that would be added.

I am wondering if it would make sense for faucet-pipeline-js to not expose the inlineDynamicImports-configuration option right now and instead make an educated decision for the user to have dynamic imports inlined by default, effectively turning code splitting support off.

This would free users from having to run into the issue first and figuring out that the $option-to-be-named needs to be set/configured.

Once code splitting support is addressed, the $option-to-be-named could be exposed 🤔

I see the downside that this might result in significant bloat but maybe that is something that could be included as a hint in the bundle bloat message.

added:

I was thinking of a message like this:

this bundle looks to be fairly big, you might want to double-check whether that's intended and consider performance implications for your users: $path

This could be caused by faucet automatically inlining dynamically imported modules. There is an open issue to address this: https://github.com/faucet-pipeline/faucet-pipeline-js/issues/119

The linked issue – or another linked issue – could elaborate on the situation and would allow users to join the discussion (and maybe more).

I think it would be quite useful – maybe even necessary – to have an option to turn this message off though. It is great to make to quite prominent in the beginning, so that users are aware of whats going on, but this type of message can become quite annoying over time.

make an educated decision for the user to have dynamic imports inlined by default

I'm not keen on this kinda magic: If people wanna use this, they really should explicitly opt in. Any attempt at guessing on our part is bound to be wrong sometimes, which just makes the situation more complicated and messy. In other words, I don't think we can/should abstract this away; users will need to understand what's going on under the hood and decide whether, effectively, they wanna change the semantics of dynamic imports for their bundles.

(I might be a little slow to respond in the coming weeks, I'm afraid.)

faucet-pipeline / faucet-pipeline-js

writeConfig: Support `inlineDynamicImports: true` #208