adds sort option #79

SeeThruHead · 2017-09-01T19:37:48Z

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 · 2017-09-01T19:42:21Z

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%)`	⬆️

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 · 2017-09-01T21:16:51Z

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 · 2017-09-02T05:04:17Z

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

allows users to sort the file list before it is passed to reduce, sorting is defaulted to dependency order using topological vertice sort on edges [parent, child] (adapted from html-webpack-plugin);

mastilver · 2017-09-04T08:55:22Z

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- · 2017-09-04T11:56:31Z

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

SeeThruHead · 2017-09-06T19:53:48Z

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

mastilver · 2017-09-08T07:44:31Z

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 · 2017-09-08T07:45:21Z

@a-x-

But we should compare new and current sorting in more cases

I did not understand this, can you be clearer?

a-x- · 2017-09-08T20:12:08Z

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

Ok, it seems so good

mastilver · 2017-09-12T09:19:18Z

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 · 2017-09-12T13:23:13Z

^ 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 · 2017-09-12T14:18:00Z

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 · 2017-09-12T15:32:36Z

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 · 2017-09-12T15:37:41Z

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 · 2017-09-12T15:48:08Z

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 · 2017-09-12T16:02:08Z

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 · 2017-09-12T16:03:20Z

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

mastilver · 2017-09-19T14:22:30Z

@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 · 2017-09-19T14:24:06Z

ok i can do that.

mastilver · 2017-09-19T20:16:41Z

suit yourself ;)

joscha · 2017-09-21T06:07:19Z

\o/ for this @SeeThruHead

mastilver · 2017-09-21T10:51:06Z

@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 · 2017-09-21T11:50:52Z

Sort option was added by ae03fbd

SeeThruHead · 2017-09-21T14:43:45Z

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

mastilver · 2017-09-21T15:47:40Z

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 · 2017-09-21T21:50:57Z

@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 · 2017-09-21T21:55:45Z

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

mastilver added the breaking changes label Sep 1, 2017

mastilver added this to the v2.x milestone Sep 1, 2017

SeeThruHead force-pushed the master branch from 348fc60 to dac9540 Compare September 2, 2017 05:27

adds sort option

c6851ba

allows users to sort the file list before it is passed to reduce, sorting is defaulted to dependency order using topological vertice sort on edges [parent, child] (adapted from html-webpack-plugin);

SeeThruHead force-pushed the master branch from dac9540 to c6851ba Compare September 2, 2017 05:29

a-x- approved these changes Sep 4, 2017

View reviewed changes

This was referenced Sep 13, 2017

Option to error if an asset is written that already exists in the seed object #82

Closed

replace reduce by generate #89

Closed

mastilver removed the breaking changes label Sep 21, 2017

mastilver removed this from the v2.x milestone Sep 21, 2017

SeeThruHead closed this Sep 21, 2017

SeeThruHead mentioned this pull request Sep 21, 2017

adds topological sort example #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds sort option #79

adds sort option #79

SeeThruHead commented Sep 1, 2017 •

edited

Loading

codecov-io commented Sep 1, 2017 •

edited

Loading

mastilver commented Sep 1, 2017

SeeThruHead commented Sep 2, 2017 •

edited

Loading

mastilver commented Sep 4, 2017

a-x- commented Sep 4, 2017

SeeThruHead commented Sep 6, 2017

mastilver commented Sep 8, 2017

mastilver commented Sep 8, 2017

a-x- commented Sep 8, 2017

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 12, 2017 •

edited

Loading

SeeThruHead commented Sep 12, 2017 •

edited

Loading

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 19, 2017

SeeThruHead commented Sep 19, 2017

mastilver commented Sep 19, 2017

joscha commented Sep 21, 2017

mastilver commented Sep 21, 2017 •

edited

Loading

mastilver commented Sep 21, 2017

SeeThruHead commented Sep 21, 2017

mastilver commented Sep 21, 2017

joscha commented Sep 21, 2017

SeeThruHead commented Sep 21, 2017

adds sort option #79

adds sort option #79

Conversation

SeeThruHead commented Sep 1, 2017 • edited Loading

codecov-io commented Sep 1, 2017 • edited Loading

Codecov Report

mastilver commented Sep 1, 2017

SeeThruHead commented Sep 2, 2017 • edited Loading

mastilver commented Sep 4, 2017

a-x- commented Sep 4, 2017

SeeThruHead commented Sep 6, 2017

mastilver commented Sep 8, 2017

mastilver commented Sep 8, 2017

a-x- commented Sep 8, 2017

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 12, 2017 • edited Loading

SeeThruHead commented Sep 12, 2017 • edited Loading

mastilver commented Sep 12, 2017

SeeThruHead commented Sep 12, 2017

mastilver commented Sep 19, 2017

SeeThruHead commented Sep 19, 2017

mastilver commented Sep 19, 2017

joscha commented Sep 21, 2017

mastilver commented Sep 21, 2017 • edited Loading

mastilver commented Sep 21, 2017

SeeThruHead commented Sep 21, 2017

mastilver commented Sep 21, 2017

joscha commented Sep 21, 2017

SeeThruHead commented Sep 21, 2017

SeeThruHead commented Sep 1, 2017 •

edited

Loading

codecov-io commented Sep 1, 2017 •

edited

Loading

SeeThruHead commented Sep 2, 2017 •

edited

Loading

mastilver commented Sep 12, 2017 •

edited

Loading

SeeThruHead commented Sep 12, 2017 •

edited

Loading

mastilver commented Sep 21, 2017 •

edited

Loading