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

[Roadmap] The future of experimentalCodeSplitting and experimentalDynamicImports #2092

Closed
16 tasks
lukastaegert opened this issue Mar 28, 2018 · 30 comments
Closed
16 tasks

Comments

@lukastaegert
Copy link
Member

lukastaegert commented Mar 28, 2018

This is a collection of ideas what should happen before we deprecate these two experimental options. Of course everything here is open to discussion. This description should be updated accordingly including issues tracking individual steps.

experimentalDynamicImports

At the moment, this option controls both the parseability of dynamic imports as well as various code snippets dealing with them. As this is still only a stage-3 feature, we should probably not get rid of this option just yet. Nevertheless, the following might make sense:

  • experimentalDynamicImports should only control if the corresponding acorn plugin is added but all other checks for this option should be removed. I.e. if the user adds their own acorn plugin to parse this, everything should still work. This will reduce some complexity for us.
  • Add an option inlineDynamicImports to control if dynamic imports are inlined.
    • This option will only be observed if a single entry point is provided; for multiple entry points, setting it to true will throw an error (or a warning that the option is ignored)
    • The default will be false. In case file is provided, the necessary dir will be the directory of the output file
    • In case the option is not provided and we have IIFE or UMD as the output format and a dynamic import is used, an error should be thrown

experimentalCodeSplitting

  • This option should be removed and be made active by default
  • Internally, there is still quite a bit of duplication between the code-splitting and non-code-splitting workflows, especially in src/rollup/index.ts but also in Graph.ts. This is problematic as improvements being made for one workflow will not automatically propagate to the other. With the recent improvements for code-splitting, it should be possible to map the non-code-splitting case to the code-splitting case and get rid of the code path of the former. If there are performance concerns, we should make sure that any additional logic in the code-splitting case executes as quickly as possible in situations where it is not necessary.
  • If input is an array of length 1 or a single entry should be treated the same way. This is important for the CLI where it is not easily possible to specify an array of length 1. Instead
  • The question where output files are generated and how they are named should be primarily decided by the presence of the dir option:
    • If dir is present, adding the file option should produce at least a warning.
    • If dir is present, using iife or umd formats should produce an error.
    • If dir is present, output file names will respect the chunkNames/entryNames options
    • If dir is missing, providing more than one input should produce an error
    • If dir is missing, providing an alias name for a single input either via CLI or input option should produce at least a warning
    • If dir is missing, providing the manualChunks option should produce an error
    • All plugin hooks should be reviewed for compatibility and if we need interface changes
      • transformBundle
    • new transformChunk hook? transformChunk hook #2043
    • other hooks?

There may be more ideas to add to this list/things to be discussed/things that were missed. Looking forward for feedback. Might make sense to add experimentalPreserveModules here as well.

@lukastaegert
Copy link
Member Author

To be discussed: How to handle IIFE and UMD builds.

@lukastaegert
Copy link
Member Author

I guess the easiest approach it so just forbid using those with the dir option.

@guybedford
Copy link
Contributor

guybedford commented Mar 29, 2018

One thing I would like to work on before unflagging is reviewing all the plugin hooks against this again, including transformBundle and if we can do a transformChunk (#2043). Will aim to come back to this one in the next couple of weeks.

@guybedford
Copy link
Contributor

guybedford commented Mar 29, 2018

Also basic dynamic import interpolations (#2097) would be good to include (import(path/${variable}')`, where effectively globbing is applied in detecting entry points.

@lukastaegert
Copy link
Member Author

One thing I would like to work on before unflagging is reviewing all the plugin hooks against this again

Good thinking, added to the list

Also basic dynamic import interpolations would be good to include

I disagree. I think this should be an optional feature and I would prefer to focus on essential, hard-to-change-later aspects here. The list is already very long as it is and I think we should rather focus on putting the final touches to the core parts first.

@guybedford
Copy link
Contributor

Perhaps the input array / string distinction is just a little too non-obvious.

Thinking about #2123 further, I'm wondering if we shouldn't have a boolean inputOptions.codeSplitting which is used to turn code splitting on or off.

This would simplify a lot of the logic knowing what type of build we are doing.

We could then make an automatic determination of the codeSplitting option based on multiple inputs, an input array, or an input object defaulting to enabling code splitting. In the CLI a single input would still be a non-code splitting build so a --codeSplitting flag would need to be added for a single file input that wants its dynamic imports split out.

@lukastaegert
Copy link
Member Author

My hope is that in the end, the non-code-splitting case will actually internally be handled as a code-splitting case but with a single chunk and a different chunk-naming logic that uses file instead of dir. I.e. we do not need a flag to "enable" or "disable" code-splitting but a flag to tell us where the output is placed and how it is named.

If dynamic imports are handled separately via their own option inlineDynamicImports, is there anything I overlook here to prevent this from being possible?

@guybedford
Copy link
Contributor

The other issue here is that the output of bundle.write or bundle.generate would have to be keyed by the input.file in the single-file build case even though there is only one output. There might be some normalization complexities here meaning result[input.file] can't reliable be used as the index, such as if input.file were an absolute, relative or unnormalized path.

I think if we can find a good way to handle this bundle return, then yes inlineDynamicImports would be enough.

@guybedford
Copy link
Contributor

An important update here - inlineDynamicImports only ever applies to single-file builds so I think I would still rather treat codeSplitting as a flag here that is by default on for an input array or object.

This means that instead of rollup.rollup({ input: 'x', inlineDynamicImports: false }), that workflow would become rollup.rollup({ input: 'x', codeSplitting: true }), and this is the only real scenario in which you'd use that option as it would apply by default for multiple inputs.

I'm in the process of reworking the above into a single rollup build code path though, and will be combining this with the plugin API changes PR.

@lukastaegert
Copy link
Member Author

inlineDynamicImports only ever applies to single-file builds

But why should inlineDynamicImports only be useful when non-codesplitting? I could envision code-splitting scenarios where this could still have some value, for instance when bundling for a target that does not support dynamic import but you still want code-splitting (for instance to have a separate vendor bundle that does not change as often).

Otherwise those options are probably equivalent.

@lukastaegert
Copy link
Member Author

I'm in the process of reworking the above into a single rollup build code path though, and will be combining this with the plugin API changes PR

Sounds awesome!

@guybedford
Copy link
Contributor

@lukastaegert firstly, all targets except globals support dynamic imports, and code-splitting is explicitly disabled for IIFE and UMD targets still entirely.

Secondly, there is currently no logic to inline dynamic imports as if they were static imports (eg if two modules dynamic import import the same shared module). Rather the inlining logic is based on inlining the namespace for a module already in the chunk.

I guess we could add some new logic to convert dynamic imports into static imports that resolve into promise expressions, but I'm really not sure what use case that would be useful for to justify the development on that.

Note that simply defining a manual chunk with the contents of the dynamic import will effectively defer to the vendor chunk already, with the benefit that the vendor chunk doesn't need to be loaded as a static dependency if it isn't used statically.

@lukastaegert
Copy link
Member Author

I guess you are right in that the implications are probably not worth the effort (+ the additional formats supported in the single file build). So I’m ok with going along with your suggestion, thanks for taking the time to think about mine 😉 Especially since this flag will be rather invisible i.e. only needed when you actually want code splitting when starting with a single entry point.

That being said, I would actually expect everyone who uses dynamic imports to want code-splitting so we might think of tweaking the default in the single file case e.g. depending on the presence of the dir option or the output, otherwise people might find the behaviour confusing. Since dir is an output option it would be interesting to know if your approach would support this.

@guybedford
Copy link
Contributor

We could definitely consider switching to have codeSplitting the default. Perhaps inlineDynamicImports can be an option specifically only for single-file builds if we did that?

I've actually reworked the logic already (pending PR update) around handling output.file as setting the dir value to dirname(output.file) and supporting code splitting fine through that option for eg dynamic import entry points. Then throwing if you provide multiple inputs with file only.

Will hopefully be able to sync the PR by the end of this week with these updates.

@lukastaegert
Copy link
Member Author

I've actually reworked the logic already (pending PR update) around handling output.file as setting the dir value to dirname(output.file)

Ah I see! Not what I had in mind originally but this sounds like a really good idea and will definitely lower the barrier to use code-splitting!

@lukastaegert
Copy link
Member Author

I have somewhat updated the description but the code-splitting section probably needs a little more work

@BrainBacon
Copy link

Quick question regarding code-splitting; is there a way to input an entire directory?

I currently have a directory structure as follows:

src
├── dir1
│   └── index.js
└──  dir2
    └── index.js

With my rollup config, if I have an input array as follows:

const input = ['./src/dir1/index.js', './src/dir2/index.js'];

With the dir output option specified to lib It results in the following:

lib
├── index.js
└── index2.js

This is an undesirable result since I want consumers of the library to be able to do the following in cjs:

const Dir1 = require('lib/dir1');

@guybedford
Copy link
Contributor

@BrainBacon this is a great question, and something we're still working out how to handle best, so appreciated for the feedback.

Currently the way to do this would be via input: { 'lib/dir1': './src/dir1/index.js', 'lib/dir2': './src/dir2/index.js' }.

That said, it could be nicer of the name guess was better than just path.basename(input). Perhaps there would be room for a inputNameBase to associate with the input string and input array cases. Not sure how cryptic all the variations become then though? Open to suggestions here.

@guybedford
Copy link
Contributor

guybedford commented May 1, 2018

Another option here could simply be to improve the name guess from basename(input) to basename(input) === 'index.js' ? basename(dirname(input)) : basename(input), which seems like it might work well for your case. I guess it depends how much we want to double down on multiple entry point imports being a flat require namespace (most packages want import "pkg/name" not import "pkg/sub/dir/name") for brevity. Then leaving the object form for the more complex manual cases only.

@BrainBacon
Copy link

@guybedford Thank you so much!

Hmm yeah this is a complicated problem. My intuition was that if I could specify a directory as the input, then the output dir option would preserve the directory structure and file names I provided with code-split dependencies created in the root of the output directory.

I don't know if that helps, but maybe a list of use-case examples can help with working through the problem?

@BrainBacon
Copy link

BrainBacon commented May 1, 2018

@guybedford

"from basename(input) to basename(input) === 'index.js' ? basename(dirname(input)) : basename(input)

In my case this would also result in undesirable behavior since some of the directories have multiple JS files in addition to the index.js file.

I can see the following situation ending up breaking that system:

src
├── foo
│   ├── bar.js
│   └── index.js
└── bar
    └── index.js

Would result in:

lib
├── foo.js
├── bar.js
└── bar2.js

@guybedford
Copy link
Contributor

@BrainBacon agreed, perhaps just using the input object form is the better approach here?

@lukastaegert I've been thinking a little more about the code splitting use of input with regards to #1435 and I'm now getting worried users might completely misinterpret what multiple inputs mean - thinking they are building multiple files, not multiple entry points.

Perhaps we should make the code splitting distinction that input is for a single entrypoint, and that entryPoints can be a new option for code splitting rather (now that we have this term available again). That way an array input could mean building the module import "a"; import "b';. would be interested to hear your thoughts on this one.

@lukastaegert
Copy link
Member Author

Perhaps we should make the code splitting distinction that input is for a single entrypoint, and that entryPoints can be a new option for code splitting rather

I guess you are right, rollup IS getting confusing. There seems to be some interest in such a feature.

I think the idea that people have is that having input as an array will just build independent bundles using the same options and some sensible naming logic and place the results in dir. And it DOES make sense and is closer to the use cases of library developers who are still some of our most important users.

Maybe at the same time we get rid of the array form for code-splitting as the object form is superior in most ways?

@guybedford
Copy link
Contributor

@lukastaegert it seems I actually misinterpreted the issue at #1435 in thinking this was building a concatenation of the inputs - instead it seems that the expectation users have around multiple inputs is that they are separate input builds, run one after another.

What I would say is that code splitting is in many cases optimal for the use case of many separate input builds (being different to separate output builds for different target environments for example).

Code splitting solves the same use case but with the additional feature of shared bundles, which I think might actually a better default for the same use case.

That said, we could possibly still have a boolean codesplitting option to switch into a code splitting mode explicitly instead of automatically, even for multiple inputs.

I also find an input array very useful myself in bundling simple multiple entrypoint cases as you can just do rollup src/x.js src/y.js.

So I'm tempted to say we leave things as they are with the input after all... will aim to summarize my thoughts in #2126 further with the updates there along these lines.

@CoskunSunali
Copy link

I think the idea that people have is that having input as an array will just build independent bundles using the same options and some sensible naming logic and place the results in dir. And it DOES make sense and is closer to the use cases of library developers who are still some of our most important users.

@lukastaegert Exactly this! Developing libraries that expose components which can then be loaded individually by requiring the components asynchronously.

I am currently using the multiEntry plugin but an out of the box implementation with some nice documentation would be very much appreciated.

By the way, yes, things are possible when you use the multiEntry plugin but it is still not at its best.

I am still struggling to figure out how to bundle "common" scripts to those individual components for example.

@lukastaegert
Copy link
Member Author

I am still struggling to figure out how to bundle "common" scripts to those individual components for example

But this actually sounds like @guybedford's suggestion is more like what you want. Just for reference, these are the two options explained with a small example:

Assume you have modules

  • a.js imports c.js
  • b.js imports c.js
  • c.js imports d.js

Following my suggestion, rollup a.js b.js --format cjs --dir out would place the following files in out

  • a.js containing the original a.js, c.js, d.js
  • b.js containing the original b.js, c.js, d.js

So you see there is an overlap between the generated files. @guybedford's suggestion, on the other hand, would produce something like the following result:

  • chunk-xxx.js containing the original c.js and d.js
  • a.js containing the original a.js and an import of chunk-xxx.js
  • b.js containing the original b.js and an import of chunk-xxx.js

So the shared dependencies would be automatically extracted in a separate bundle. The current question is if this should be the default behaviour.

@CoskunSunali
Copy link

CoskunSunali commented May 20, 2018

Thanks for the comments @lukastaegert!

I guess what I am trying to achieve is even something different.

Assume you have modules

  • a.js imports c.js
  • b.js imports c.js
  • c.js imports d.js

I would like to invoke rollup a.js b.js c.js d.js --format cjs --dir out and would like to have the following files in out.

  • a.js
  • b.js
  • c.js
  • d.js

I already know (since I am the one developing the library) that the entry to the application is via a.js and consider the following semantic code:

SystemJS.import('out/a.js').then((aModule) => {
    aModule.bootstrapApplication();
});

I assume SystemJS should be smart enough to load the following files in this case.

  • c.js because it is imported by a.js
  • d.js because it is imported by c.js

b.js is so far out of the scope and is not needed for the application. Consider it as convenience script. It is not required for the application to work but 3rd party developers can develop plugins that needs it and it should be loaded the moment a 3rd party plugin requires it.

Imagine now some application plugin has been developed by a 3rd party. They know that there exists a b.js they have to extend/use when they are developing certain functionality.

What is important here is that the 3rd party script (let's name it 3.js) does not know anything about rollup, a.js, c.js and d.js. It only knows about b.js. 3.js does not even get processed by rollup. It is fully 3rd party, developed as a system module.

a.js has some functionality that supports loading 3rd party plugins using the following code.

aModule.loadPlugin('3.js');

loadPlugin method knows that it should call SystemJS.import for 3.js. 3.js has the following code in it.

System.define(['out/b.js']...

So SystemJS should also load b.js and that should be pretty much that.

I partially implemented what is being said. Just had to hack it too much here and there and don't feel very comfortable.

I spent days with gulp, achieved what I wanted but was not comfortable with its output. I spent days with webpack, partially achieved what I wanted but the output is terrible.

Rollup, I have to be honest here, produces the best output so far but I have the following impressions.

  • Rollup assumes that it is a bundler for javascript files, since it calls itself as ES6 bundler. But I wish not because it is such a nice tool, it should call itself as a bundler instead of ES6 bundler and let us do whatever we want with the nice architecture and plugin system you have here. ES6 bundler? I believe that should be a part of the imaginary rollup-plugin-es6 plugin.
  • The documentation is definitely not developed enough. That goes for both core rollup and plugins. Especially with rollup core, google always shows results from the github wiki but that wiki does not exist anymore.
  • It should not be so hard to copy some assets from source to destination when using rollup. We should be able to say I handled this type of file, you don't have to do anything further to rollup within transform method. Currently we have to return an empty string as code, otherwise it tries to transpile it and considers it a JS file even though it is just some asset I want to copy to the output.

That being said, I have to say thank you for your efforts on open sourcing rollup and spending your valuable time to fix it, improve it, release it.

@lukastaegert
Copy link
Member Author

The documentation is definitely not developed enough

I can definitely feel with you here. The problem is that at the moment, we are basically two people taking care of rollup core and some people who kindly took responsibility to maintain some of the plugins but there is noone looking after the documentation. At the moment, @guybedford and myself try to keep the info mostly up to date. But if there is anyone willing to do improvements in this area, this would be more than welcome. And documentation is an easy way to start contributing.

Especially with rollup core, google always shows results from the github wiki but that wiki does not exist anymore

I wonder why, I hope this will improve over time.

it should call itself as a bundler instead of ES6 bundler

Unfortunately, this is not some extravagance but there is a reason for this. Rollup was built around ES modules not only because it is the only "official" JS module format but also because it is the only format that enables us to do the deep analysis and optimizations Rollup is known for. Internally, everything is represented as ES6 modules and I fear there is no easy way to change this as other formats have very different semantics. Of course, only time will tell where Rollup is heading here.

It should not be so hard to copy some assets from source to destination when using rollup

I guess this could easily be improved, maybe it would be worth creating a separate issue for this. Beyond that, @guybedford is working on a new API to emit files but I did not take a look yet in how far this extends to plugins yet.

Thanks for the feedback!

@NeoLegends
Copy link

It should not be so hard to copy some assets from source to destination when using rollup

I like that rollup is currently limited to bundling JS and does not try to be a full build system like webpack. IMO you should just use gulp or some other build system for these kinds of tasks.

@lukastaegert
Copy link
Member Author

I think most of this has been fledged out now and should be part of the next major release. Feel free to reopen if there is more need for discussion.

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

5 participants