Modify logic for deciding to use "dir" vs "file" in rollup output options.

[geoffpotter]

Information: Modify the logic for deciding to use dir vs file in rollups output options.
Kind: bugfix? maybe feature.

Description: I'm not sure if this is a bug or not, but I've been unable to get this plugin to use the "dir" output option instead of "file", it seemed to me that files.src.length was always one, regardless of how many source files I used, or how I formatted my files section of the config. I left this code in place, in case it's not broken and I just lack the proper understanding of how to do it.

Even if this part isn't a bug, I figured we could use some extra options in this area, so I added an options entry to force it to DIR, and tried a different auto detect method: looking for a dot in the destination path to see if it's a file, if it's a directory, then use DIR. This makes way more sense to me than changing the output options based on the number of input files.

Not sure if it matters what I'm trying to do with it, but this change allows me to easily use Rollups code splitting features. Here's my relevant config, with the current version this would cause an error, but with this change it works:

    rollup: {
      options: {
        plugins: ...,
        manualChunks(id) {
          if (id.includes('node_modules')) {
            return 'vendor';
          }
          if (id.includes("subModule")) {
            return "subModule";
          }
        }
      },
      main: {
        files: [{
          expand: true,
          cwd: 'build/flattened/',
          src: ['main.js'],
          dest: 'build/rolluped/',
          filter: 'isFile',
        }],
      },
    },

[GMartigny]

Hi,

Thanks for the PR. Before discussing solutions, I need to better understand you pain point.
Why are you using the expand option on one file only ?

When you define files to Grunt, you either have one input file (then Grunt expect the output to be a file path) or you have multiple input files (then Grunt expect the output to be a directory path).
I don't think this plugin should override this behavior, as it would be unexpected by most users.

In your example, you should either have more than one entry in src (or use glob pattern), or set dest to a file path.

[geoffpotter]

Background on what I'm doing: playing a game. Screeps to be specific. It's a node 10ish environment running inside the isolatedVMs project. So there are various limitations I have to deal with that others may not, cuz I can't change my runtime environment.

But, please ignore that I'm "just playing a game", I'm pretty sure this stuff needs to work better for other people in non standard, but not a game, situations. Although, I would agree it needs to not disturb "normal" users who are doing real work. My goal is very much not to disturb them, while letting me use this plugin. It gets me closer than anything else I've tried to what I want to do.

Additionally, the plugin works fine for my game, as long as I stick to basic usage, and basically just avoid using a dir output for rollup or any features that require it. But, for reasons that shouldn't matter while making build tools, I want to use code splitting and have different output bundles, I can't use rollups bundling feature without using the dir option instead. I guess I could go make a pr for rollup that makes it so that instead of throwing an error when you do this, it just removes the filename from your file directive and uses it as a dir one. But then i gotta argue with those guys, and really I feel like the issue is more with this plugins inability to use the dir directive than it is rollups handling of it's options.

I'd be fairly comfortable with only adding in an explicit override that forces dir, but based on further testing this pr needs more work, so either way, don't approve yet, I have additional changes, just wanted to get this conversation started early to help figure out what I need to write to fully solve the issue for normal people plus weirdos like me.

But putting the override option aside for a moment, since I'm assuming that should be agreeable to you, since there's no no change unless ya set it.

I really think the logic for selecting dir is flawed. It's using the "number of INPUT files"(but not actually) to determine the use of dir or file for rollups OUTPUT, but it makes more sense to me to key off of whether the destination is a directory or a file. Additionally when you give grunt multiple input files and an output directory, it doesn't give you a files array with a count over 1, as the code expects, it gives you multiple entries in the file array, each with a single source file and a matching output dest. Basically I think the assumptions about how the files array works that were used to build the plugin were either wrong, or they've since changed. That said, I'm still figuring out how it works, so it's possible I've missed some obvious solution here.

Do you have an example config that would actually cause it to use the dir option? I added a bunch of console.logs and tried a everything I could find and the length of that array was always one, even when it was processing multiple files. If there is a way to do it, it's possible that would work for me, but at this point I'm pretty well convinced that it's at least entirely too hard to get it to use dir when you want it too, so at a bare minimum we need an explicit override, or a flag to switch to a new auto detection method, or SOMETHING. I'm happy to code the solution, whatever it is.

[geoffpotter]

I think the use of the manualChunks(id) option to rollup is important to this issue. That's how I'm splitting my chunks. You can't use this function with the file param. I can't just use multiple entry points(although, again, that doesn't even work for me) because I don't actually have multiple entries, I'm just trying to split the code up to make other things easier, and rollup will basically pack all of my code into each of the entry files so they can operate alone. My project itself only has one entry point, the "loop" function exported from src/main.js that the game calls every "tick"/frame/moment/whatever to run my virtual player.

From here on may be tl:dr
The game itself doesn't support folders or many modern features(as I said, node 10ish running inside the isolatedVMs project, we've got 1000 shitty programmers worth of JS code running in the same server, so isolation is key), I'm trying to provide the game community(that contains a lot of newbies to JS and coding) a project framework that supports a lot of different ways to do things, focusing on being able to copy in their old game code into src/ and just go. Aiming to cover most basic variations of ways to write JS + TS, different ways to bundle your code(why? I don't know, maybe they're smarter than me) into various sets of files. Npm imports. Basically just taking npm, grunt, rollup, babel, TSC + anything else I can find, and trying to make a build system that newbie players in the game can use to easily transition from the poorly written code they already have, but that at least works in the game, into a more modern JS/TS build system that also let's their current code run in the game, but gets them the ability to write all the new new cool stuff as well and transition more slowly into fully modern ES.

[GMartigny]

Ok I better understand and I manage to write a failing test. The issue is indeed with the use of manualChunks with only one input file. I'm working on a solution now and I'll reach you back when it's done.

[GMartigny]

I just published a beta (v11.9.0-0) with a fix for this. Could you confirm me that it deals with your issue ? Thanks

[geoffpotter]

I guess that solution is OK. If I find some other reason I need to use dir instead, I can use an empty function for manualChunks and that'll just act as a flag to force the plugin to pass dir instead.

Logic aside, this still doesn't quite work properly. It has a similar issue as my PR, it will add an extra directory in the output called main.js, so the path to main.js would be something like dist/main.js/main.js

As I said before, grunt doesn't ever seem to give this plugin multiple source files, and the dest property has the output filename appended to it regardless of if I give it a directory for dest.
Luckily, they do have the original values Still available, so if you change files.dest to files.orig.dest in your call to path.join, it will fix the extra directory issue.

I'll post some logs in a sec here with examples of what I'm seeing in the files array with various configs

[geoffpotter]

I added this line right after your line for useDir:
console.log("in rollup", useDir, JSON.stringify(files))

Here are the file settings:

main: {
        files: [{
          expand: true,
          cwd: 'build/step1-copy/',
          src: ['main.js', "inlineModule.js"],
          dest: 'build/step2-rolluped/',
         // filter: 'isFile',
        },],
      }

And here are the logs I get:

in rollup false {"src":["build/step1-copy/main.js"],"dest":"build/step2-rolluped/main.js","orig":{"expand":true,"cwd":"build/step1-copy/","src":["main.js","inlineModule.js"],"dest":"build/step2-rolluped/"}}
in rollup false {"src":["build/step1-copy/inlineModule.js"],"dest":"build/step2-rolluped/inlineModule.js","orig":{"expand":true,"cwd":"build/step1-copy/","src":["main.js","inlineModule.js"],"dest":"build/step2-rolluped/"}}

So even though I'm giving it a directory as output and multiple input files, all you'll see in the plugin is a single input file going to a single output file, but twice.

If I make the files array have two objects then I get the same output.

Is there some other way I'm supposed to set multiple entry points?

[geoffpotter]

Oh, by the way, for the above test I had manualChunks removed

[geoffpotter]

output.preserveModules also requires the use of dir, can we add that?

[geoffpotter]

I've confirmed that for the above config(or globs, or multiple entries in the files array), this plugin will actually run rollup multiple times, once per entry file, this will cause a bunch of issues with rollup not working as well as it should since it won't know about the other entry files. So in the above example, there's a module shared by both entry points, instead of being in a separate module, it's packed into both entry point bundles. I would also think this has an effect on build times for larger projects trying to use this plugin with many entry points, if I try to implement the feature I was using manualChunks for(any dir with a main.js file goes into it's own bundle), but try to send multiple entry points as I did above, then my build time doubles or tripples.

Here are some more logs from that setup, with the whole current.file array, timing for each call to rollup, and the entry in the files array each one is operating on.

[ { src: [ 'build/step1-copy/main.js' ],
    dest: 'build/step2-rolluped/main.js',
    filter: 'isFile',
    orig:
     { expand: true,
       cwd: 'build/step1-copy/',
       src: [Array],
       dest: 'build/step2-rolluped/',
       filter: 'isFile' } },
  { src: [ 'build/step1-copy/inlineModule.js' ],
    dest: 'build/step2-rolluped/inlineModule.js',
    filter: 'isFile',
    orig:
     { expand: true,
       cwd: 'build/step1-copy/',
       src: [Array],
       dest: 'build/step2-rolluped/',
       filter: 'isFile' } } ]
in grunt-rollup false {"src":["build/step1-copy/main.js"],"dest":"build/step2-rolluped/main.js","filter":"isFile","orig":{"expand":true,"cwd":"build/step1-copy/","src":["main.js","inlineModule.js"],"dest":"build/step2-rolluped/","filter":"isFile"}}
starting rollup [ 17971, 524870600 ]
in grunt-rollup false {"src":["build/step1-copy/inlineModule.js"],"dest":"build/step2-rolluped/inlineModule.js","filter":"isFile","orig":{"expand":true,"cwd":"build/step1-copy/","src":["main.js","inlineModule.js"],"dest":"build/step2-rolluped/","filter":"isFile"}}
starting rollup [ 17971, 525600500 ]
Writing build/step2-rolluped/inlineModule.js...OK
Rollup Execution time (hr): 0s 57.3799ms
Writing build/step2-rolluped/main.js...OK
Rollup Execution time (hr): 0s 63.5247ms

I'm not sure exactly what the solution is yet, but it seems like we'd need to preprocess the current.files array and group them by their original destination path, then loop through that and use the src from all the matching original entries as entry points to rollup. Basically change the current.files array to work the way the code already assumes it does