Should onwrite also receive the chunkName as a parameter somewhere? Previously in "single bundle mode", onwrite could figure out what files were written using the outputOptions, but now with chunks, I think it needs the chunkName to be able to re-construct the written file paths.

The chunk object above actually does include the name as well, so I believe that would be passed to the plugins ok?

Ah okay, must've misread it. Sounds good!

When the codeSplitting flag is true then the TS types returned by generate will be different. Currently TS believes that the same types are returned whether this is set or not. I am not exactly sure the right approach to resolve this rather than setting an OR statement in the OutputBundle's generate result.

There are basically two overloads of the generic rollup function from a typing perspective:

interface SingleInputOptions extends InputOptions {
  input: string;
}
interface MultipleInputOptions extends InputOptions {
  input: string[];
}
function rollup(options: SingleInputOptions): Promise<BundleSingle>;
function rollup(options: MultipleInputOptions): Promise<BundleChunks>;

Cool this would work as well.

Better describe purpose of this specific test?

Better describe purpose of this specific test?

Better describe purpose of this specific test?

Better describe purpose of this specific test?

Better describe purpose of this specific test?

So I assume the point of this test is to show that the unused imports from dep2.js are properly removed. But maybe we can do better by not emitting this chunk at all? But this might also be a possible future enhancement.

For instance the logic could be that a chunk is not emitted when

• There are no synchronous imports from it (otherwise we should probably include it for potential side-effects until we found away to improve the side-effect detection logic to find out if a chunk has side-effects in its initialization code. Actually, this could probably be done by checking if any of the top-level statements are included or if one of the imports has side-effects. But again, maybe a future enhancement)
• None of the dynamic imports are included

Another interesting fact I stumbled upon that we might improve in the future is that when

• a chunk exports a variable and
• this chunk is imported by two other chunks both of which import this variable and
• only one of the chunks actually uses this variable,
  the imported variable is retained for both chunks. The reason is that there is only one variable entity and as soon as it is included, all imports of this variable are included. If none of the chunks use the variable, it is properly removed. I think it is more of a cosmetic issue but still.

You've exactly understood the two limitations of the approach I put together - the chunking algorithm is run on the graph before treeshaking, and the export boundaries between chunks aren't properly pruned. I originally aimed to handle both of these cases (and my first few attempts were in this direction), but hit some road blocks.

When running chunking on the graph after treeshaking, execution order still has to be maintained as an invariant. In addition we need to work out which chunks can be considered pure in order to skip emission like you mention.

If I remember correctly, the main issue I hit when implementing this (after fixing up export tracing etc etc) was that the concept of "marked" or "used" must then be considered uniquely for each specific chunk, which wasn't a simple capability to add. I can't remember what the other issues were, but if you're interested I could try trawling through my notes for future reference.

Note that in many of my practical investigations of the algorithm, these assumptions ended up not seeming so bad at all though. The majority of cases work really well without these features. I think as we use this feature we'll get a better idea for how bad "pure" chunk relations and unpruned boundaries might end up looking, and can either consider specific fixes, or taking the more general route then.

I guess indeed the proper term should be facade without the "s" in both the description and folder name.

It seems this could be simplified to only handle the error once and merge conditions, i.e. handle the error if command._.length >= 1 and then do the rest in a nested if clause.

When building rollup, these imports create a lot of noise and especially the path, buffer-xor and crypto imports break browser compatibility (which is really important for the REPL to work; note that we cannot have unresolved dependencies for the browser build)!

You will also notice there is another import of path related functionality from ./utils/path further up. In a browser build, this import is replaced via an ad-hoc plugin in /rollup.config.js with a polyfill in /browser/path.ts. I guess a similar route should be taken for the other path-related methods or otherwise we should consider switching to rollup-plugin-node-builtins which promises to also solve the buffer related problems that npm run build lists.

For crypto, maybe a browser-compatible drop-in can be found that we might swap in similar to the path approach? Otherwise, as you are basically only using the hashing and the xor, maybe a more specialised library can be found?

Noting that, maybe we need a test for the browser build that at least checks for externals or unresolved dependencies.

Sure will see what I can do.

Ok I've replaced it with native functions here and gone with numbered chunks. Hashing could be useful, but it should at least be proper hashing of the source that can cover all file names in the build as a separate feature.

There is a lot of overlap between buildSingle and this function that should probably be extracted if we want to keep both functions; though of course it would be even better if buildChunks could just handle the buildSingle case as well if we slightly modify it.

Also this function is really long. It think brain-overload-wise, but also with regard to separation of concerns, it would benefit if the final chunk preparation could be extracted. Doing this first might also help us to determine more easily if the non-chunking case could become a special case of the chunking version. But maybe this does not make sense as you have obviously put much more thought into this. Still, some splitting up of this >120 lines method certainly would not hurt maintainability.

At it's made an improvement on the buildSingle function though from where it was before as the main build method.

I'm still weary of over-abstracting though, it can be just as much or even more of a maintenance issue having to jump around a file to work out exactly what a method is doing. Seeing the low-level details in one place is a powerful way to understand something even if it does mean you have to stare at it for longer than 5 minutes :P

Also the idea is the code comments provide the high-levels... reading through the comments gives an algorithmic overview, better than some clever method names might.

First a promise as this is going to be a little longer (but only this one time): This is the last time for this review that I will try to make a point that smaller functions are a gods-end for maintainability and I will happily merge this PR without any changes here!

I also want to stress that I have the deepest respect for you as well as the tremendous work you did here. Nevertheless, I like to hold it like this tweet and hope others do towards me and my code as well: https://twitter.com/wolframkriesing/status/951375642876145664 Especially I want to obey commandment 8 in the given link, as hard as it feels.

That being said, I really do hope to eventually be able to convince you about the value of smaller, well-named functions. Maybe not for this PR but for PRs to come.

Code is read many more times than it is written

When I develop new functionality, I too find it easier to write a monolithic function first because it enables me to see everything together, find parallels, etc. But this is the initial development and I am the person who developed the code so I really know my way around. But other people will need to read and understand my code. And for each of them, it will be at time and again these 5 Minutes of staring at the code. Only to find out that most of it is irrelevant for their problem. And this is not just others but my future self as well after a few months. Estimates are of a ratio of 1-to-10 writing vs. reading, but it surely depends on the code. I would expect this rate to be even higher for such a high-profile OSS project.

This is an excellently sums it up. If you only want to read one thing, reads this: http://sam-koblenski.blogspot.de/2014/01/functions-should-be-short-and-sweet-but.html

The important argument for readability here is the "drilling down" aspect that allows you to get to the relevant parts quickly. Modern IDEs let you jump to function and method definitions with a single click, and if you really need to see it in context, inlining a function or method is usually just a keyboard shortcut away. As well as extracting it.

Long functions make people wary of refactorings

It is much harder in a long function to switch out part of the functionality. Because variables have long scopes and there are interdependencies between different parts of the function that make people wary. So instead of just switching out the part that needs switching, people start piling up additional functionality. Because they always need to understand the whole function to be able to change the relevant part and this costs time and greatly increases cognitive overload. See here: https://www.sandimetz.com/blog/2017/6/1/the-half-life-of-code

Code that isn't easy to replace doesn't get replaced, instead it gets expanded

Comments are a bad way of documenting code

This is thoroughly described here: https://visualstudiomagazine.com/articles/2013/06/01/roc-rocks.aspx

But the real problem is that, even if that miracle of accuracy and completeness were achieved, there's no way for someone reading the comments to know that accuracy and completeness have been achieved

A much shorter to-the-point summary of how to write self-documenting code (which is what I strongly advocate!) can be found here: https://gist.github.com/tharumax/a78883d198217ec3cdaf

Maintainability is key for rollup to be ahead of the competition

I have spent several years coding in tightly integrated teams with highly talented and experienced colleagues, constant code reviews and high enforced standards of software craftsmanship. What I am proposing is not solely my personal feeling but also the result of countless years of experience of many software developers on how to keep development speed high and bugs short-lived when coding in a team as the complexity of an application grows. Even though not strictly about JavaScript, Clean Code is still one of the most comprehensive books about the issue, listing countless examples and counter-examples as well as more background to understand the value these concepts have for efficient software development. And I have myself seen over and over that even the most convinced proponents of longer functions at some point accepted the wisdom of better abstraction.
Admittedly, good function names are key when extracting functionality. But in contrast to comments, function names can easily be adjusted via your IDE (and they should be). And a reviewer will always be happy to point out where a name is mis-leading so that a better one can be found.

But now I want to be silent, finish the review and obey commandment 8.

If you provide the entryChunkNames as a parameter, this function could be extracted out of this method (and even out of this class), which if feel could help readability.

Maybe there is an if-condition missing? Because if you have this situation:

• A is the only entry point
• A imports B
• B imports A

then two separate chunks will be created for no reason at all since B could easily be included into the common AB chunk. Maybe you want this second xor to only happen if the module is different from the module with which you started the current iteration (e.g. by providing it via a second parameter)? Or am I missing something?

Also, I discovered an issue that may or may not be related to this for which I will create a test PR to illustrate it where rollup is not properly bundling an entry point.

Excellent catch - yes we must avoid xoring with itself!

Ok changed the algorithm slightly :)

Nice idea with the execution order!

As I understand it in its current implementation, code splitting is a different mode of operation for rollup as with code-splitting

• the dir property is required (even if you only specify one entry point)
• the file property is ignored in favour of the original file name (+ possible deconflicting)

If we want to keep it like that, I would suggest to remove the "experimental" from the flag. However I would actually prefer to not have a second mode of operation (and keep the "experimental" in the first iteration). What if instead it worked like this:

• If you only provide one entry point (either like "input": "./main.js" or "input": ["./main.js"]), then
  • If you only provide "output.file" but not "output.dir", this file is used as the bundle file as in this situation, there will always be a single output file (or am I wrong?)
  • If you only provide "output.dir" but not "output.file", the same naming scheme as in the chunking version of the algorithm will be used
  • If you provide both, a warning is displayed that you cannot provide both "output.file" and "output.dir" and the latter will be ignored (or the other way around, I am not partial here)
• If you provide several entry points and "output.dir" is provided, rollup will run the chunking version of the algorithm and use the original file names as a basis for the generated ones
  • If you additionally provide "output.file", a warning is displayed that "output.file" will be ignored for multiple entry points
  • If you do not provide "output.dir", an error is thrown (as is currently the case, but only if you actually have multiple entry points)

What do you think? Or do you think this is too complicated for the first iteration?

The difference here is that dynamic imports are inlined into one file for the single-file output, while dynamic imports can generate chunks for the directory output.

So I think it's perhaps clearer to consider input[], dir and input, file as completely distinct cases. We could consider allowing input as a string for the dir case possibly.

Would be nice to have a test with actual sourcemaps but if course it is kind of hard to verify they are correct. Not sure about this, commented code tends to become obsolete quickly. Maybe we check sourcemaps if they are present in the _expected folder and just add them to the basic test?

Got this going :)

As the code below does not seem to depend on curEntryChunkNames being an array, maybe we should replace it with a Set (or a blank() object) to slightly improve lookup performance when there are many chunks (i.e. Set.has instead of Array.indexOf)?

Btw. I love the new chunk names! Looks much better!

👍

My hope is that if we can manage to make code-splitting always on (as outlined above), we could basically remove the first version of generate.

Perhaps a middle ground for always-on could be that one input and an output file would output chunks in the same folder as the output file, possibly separating the dynamic import inlining option as inlineDynamicImports: true or similar?

Ah I see, I was forgetting about this. I guess a flag would definitely make sense as otherwise the behaviour would be inconsistent.

So maybe:

• If we have a single entry point
  • if output.file is provided, it is the name of the chunk generated for the entry point
  • if not but we have output.dir, we use the new naming algorithm
  • if neither is provided, we show an error that either is missing
  • if a dynamic import is encountered + inlineDynamicImports: true, it is of course inlined
  • if a dynamic import is encountered + inlineDynamicImports: false
    • if output.dir is provided, additional generated chunks are placed there
    • otherwise, an error is shown
• If we have multiple entry points
  • if output.dir is missing, we show an error
  • if output.file is provided, we show a warning that it is ignored
    ?

I noticed the experimentalCodeSplitting flag isn't really behaving like an "experimental" flag here since it's guiding the whole process. I've made the following change for now:

• If input is a string, do a single file build, assuming file and warning otherwise
• If input is an array and the flag is enabled, do a code splitting build, assuming dir and warning otherwise

We can potentially look at different arrangements of this before flag removal, but at least the flag itself is not part of the final decision now.

Very good, also like the stream-lined names!