lukeed / taskr

A fast, concurrency-focused task automation tool.
MIT License
2.53k stars 74 forks source link

Concat doesn't keep order that it was passed #165

Closed tjbenton closed 8 years ago

tjbenton commented 8 years ago

I'm having a big issue with the concat function not keeping the order that I pass. This is very big issue because I have to load the scripts in the order that I passed to them in order for them to work.

Here's the code that I'm using to try and create a combined.js file.

export async function combinedjs() {
  await this
    .source([
      'node_modules/jquery/dist/jquery.min.js',
      'node_modules/bootstrap/dist/js/bootstrap.min.js',
      path.join(app.js, 'plugins', '_*.js')
    ])
    .concat('combined.js')
    .target(dest.js);
}

The problem is that I'm trying to load jquery then bootstrap then any custom plugins that are authored. However the output of the concat file is inconsistent because it's based on when the async function is finished. So if one file in the source loads faster than another it will be added to the combined.js file first.

Sometimes jQuery loads first, sometimes Bootstrap, and sometimes one of the plugins does.

lukeed commented 8 years ago

👍 I ran into this a few weeks ago, been on my list but haven't had time to conjure/implement a quick fix.

@bucaran I'm thinking of a source() options object, which would be optional, just like how it is for target() and start()

yield this.source(src, { obj })

This specific problem could be handled as obj.order: boolean or obj.parallel, but that could be misinterpreted for start()'s parallel config parameter.

tjbenton commented 8 years ago

In the mean time if someone needs to do this before this is fixed they can use the following script I wrote. There's no external dependancies needed, it's just using what's already is install with flyjs.

import ConcatWithSourcemaps from 'concat-with-sourcemaps';
import { read, write } from 'fly/lib/utils';
import globby from 'globby';
import path from 'path'
async function concat(files, output = 'dist/js/combined.js', sep = '\n\n') {
  const parsed = path.parse(output);
  files = await globby(files);
  let content = files.map(read);
  content = await Promise.all(content);

  const cat = new ConcatWithSourcemaps(true, parsed.base, sep);

  for (var i = 0; i < files.length; i++) {
    cat.add(files[i], content[i]);
  }

  const sourceMapOut = path.join(parsed.dir, `${parsed.name}.map.json`);
  content = cat.content + sep + '// @ sourceMappingURL=' + path.basename(sourceMapOut);

  await Promise.all([
    write(output, content),
    write(sourceMapOut, cat.sourceMap)
  ]);
}

// example 
export async function combinedjs() {
  var files = [
    'node_modules/jquery/dist/jquery.min.js',
    'node_modules/bootstrap/dist/js/bootstrap.min.js',
    path.join(app.js, 'plugins', '_*.js')
  ];
  await concat(files, 'dist/lib/js/combined.js');
}
tjbenton commented 8 years ago

@lukeed @bucaran I also noticed that in the current concat function it's not actually outputting the source map file or embed the sourcemap into the file

ghost commented 8 years ago

@lukeed I thought we were going to drop support for source maps in 1.0. Good thing we didn't.

@tjbenton How difficult would it be to wrap that into a PR our way :)

lukeed commented 8 years ago

@bucaran We technically did by forcibly turning them off everywhere.

@tjbenton With the exception of concat, every other task that could use a sourcemap is almost always handled by the external library, and they do a better job that what ours did.

I suppose we could reenable them as an option on concat only, defaulting to false.