adds sort option

[SeeThruHead]

Hello! Awesome plugin you've got here. I was needing a way to have a sorted by dependency manifest (like how html-webpack-plugin works) while I was looking around i found that i could adapt your plugin to do what I needed without affecting it's functionality in any way. (by passing

  const manifestGenerator = (manifest, { name, path }) => [...manifest, path];

{
  seed: [],
  reduce: manifestGenerator,
  sort: x => -1
}

to the plugin, which gives me an array of filenames sorted by dependency.

I was hoping you might accept this PR

---

allows users to sort the file list before it is passed to reduce, can disable aphabetical sorting by passing () => -1

array passed to sort is presorted based on the dependency graph using topological sort on [parentChunk, chunk], this was adapted from html-webpack-plugin

[codecov-io]

Codecov Report

Merging #79 into master will increase coverage by 1.42%. The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #79      +/-   ##
========================================
+ Coverage   98.57%   100%   +1.42%     
========================================
  Files           2      2              
  Lines          70     78       +8     
========================================
+ Hits           69     78       +9     
+ Misses          1      0       -1

Impacted Files	Coverage Δ
lib/plugin.js	100% <100%> (+1.44%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update de15a87...c6851ba. Read the comment docs.

[mastilver]

Hi thank you

That's something I was planning to do

Can you explain me the topsort ? Also just remove the original sort (I only kept it for backward compatibility) and add a test

[SeeThruHead]

i added a test for sort which is working fine i'm having trouble setting up the entry prop to create a parent child chunk relationship though, it works for my local project, but i can't seem to get it to work with the fictures.

EDIT: ok i figured it out and added a test for dependency sorting as well

---

topological sort is a sorting of vertices on a directed acyclic graph such that for or every directed edge uv, vertex u comes before v

if your edges are parent child relationships u being parents, and v being children, if you topologically sort these edges you edge up with an order of parents always being before their children, which is what is required for including dependent js files in order on a webpage

[mastilver]

I'm not yet on board on the top sorts part

Do you think it could be possible to export that into a orderHelpers.js file so the user can pick it if they need it without bloating the plugin logic

@a-x- thoughts?

[a-x-]

I think that this logic should be in the main codebase. But we should compare new and current sorting in more cases

[SeeThruHead]

is there a consensus? should I be doing any more work on this or is this good to merge?

[mastilver]

My reasoning behind removing ordering was that nobody is using it (array output was only added on the latest release one month ago and this plugin has been around for years)

So now with this PR, rather than having thing more lightweight by removing the order, we add something that barely nobody is going to use and is relatively heavy

I prefer the helper approach as user can opt-in if they need it You mention html-webpack-plugin, I'm one of its maintainer and we keep getting request to change the order specs, so having something separated would prevent this kind of issues ;)

[mastilver]

@a-x-

But we should compare new and current sorting in more cases

I did not understand this, can you be clearer?

[a-x-]

helper approach as user can opt-in if they need it

Ok, it seems so good

[mastilver]

I went through a lot of thinking this week-end

I'm starting to think it was a mistake introducing reduce options, it's not super convenient to use

In the current state of the plugin, @SeeThruHead changes can't be extracted out, I was thinking we should replace reduce by another options, that would take seed and files, the default would be:

<option name>: (seed, files) => {
  return files.reduce((manifest, {name, path}) => ({...manifest, [name]: path}));
}

That way @SeeThruHead will have access to the full array to do it's sorting

What should we do?

• add this options: yes / no
• replace reduce by the new options / keep reduce

//cc @a-x- @gmac @zuzusik

[SeeThruHead]

^ much better than only allowing to pass the callbacks for sort and reduce, doesn't really have much to do with this PR though. this pr mainly cares about topological sorting. which still has to happen before this, unless you want to pass all the chunk data to the (seed, files) function. I can remove the sort option if you'd like.

[mastilver]

The thing is I don't think topological sorting is something that should be added...

My arguments are:

• very few peoples will actually use it (as most of them output an object rather than an array)
• In the future someone will disagree with the way it's sorted and will want to change it (it's happening at html-webpack-plugin

Although, we should allow you to do it

[SeeThruHead]

you'd need to expose a point for a person to sort chunks, which then get turned into files, internally, and then another api point to create manifests.

replacing reduce with (seed, files) => ... doesn't help me sort things based on chunk parents

[mastilver]

replacing reduce with (seed, files) => ... doesn't help me sort things based on chunk parents

I believe it will... as you will have access to files[]chunk

[SeeThruHead]

oh i see, so we have access to chunks currently in the reducer option, but we don't know when the last file is, so we can't build edges. I'd say this new replacement for reduce is a good compromise

it might be nice to have some default manifest types though. object mapping, vs ordered topological array, would be nice defaults to choose from

[mastilver]

but we don't know when the last file is, so we can't build edges.

Yeah exactly :)

I never really liked reduce, I think this new option will be better (Can't figure out a name though...)

[SeeThruHead]

it's going to be in place of manifest generation so maybe generate

[mastilver]

@SeeThruHead I'm going to merge #90 tonight (if I don't get any rejections)

Once that's done, would you mind:

• add examples/topological-sort (or whatever name you find suitable ;) ) with:
  • webpack.config.js
  • < your js files >
  • no package.json we will use the main one (devDependencies)
• add Examples section on readme (after Usage?) with one link that point to your example folder
• add .gitignore rule to ignore generated manifest: something like: examples/**/manifest.json
• keep the order options (no order should be applied when the options is not provided) and tests

After that, we will be ready to release v2 🎉 Let me know if you don't have much time on your hand, we can skip the example part ;)

[SeeThruHead]

ok i can do that.

[mastilver]

suit yourself ;)

[joscha]

\o/ for this @SeeThruHead

[mastilver]

@joscha I'm going to take care of that over lunch

@SeeThruHead I will let you take care of the example (not really urgent)

[mastilver]

Sort option was added by https://github.com/danethurber/webpack-manifest-plugin/commit/ae03fbdcd4ba8073f66efd7b97eec8df318b0c87

[SeeThruHead]

i didn't realize you still wanted sort in @mastilver :D

[mastilver]

Yeah... I'm not sure anymore if it was a good idea... :D

@joscha I'm curious how you are going to use it?

[joscha]

@seethruhead @mastilver I need to sort topologically, like the example in this PR. I think it would be best if the manifest plugin had an option to do that, but if not I am happy to use the code in this PR - however I'd still need access to the compilation, which is not covered by the current sort API?

[SeeThruHead]

@joscha sort is a basic sort, the generate option gives you full access to create your own manifest see #93