Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds topological sort example #93

Closed

Conversation

SeeThruHead
Copy link
Contributor

@SeeThruHead SeeThruHead commented Sep 21, 2017

add topological sort to new examples folder
adds examples readme entry with link to specific example
gitignore example output files

prior work: #79

@codecov-io
Copy link

codecov-io commented Sep 21, 2017

Codecov Report

Merging #93 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #93   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          78     78           
=====================================
  Hits           78     78

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 ed72f68...6f339a4. Read the comment docs.

package.json Outdated
@@ -41,7 +41,8 @@
"dependencies": {
"fs-extra": "^0.30.0",
"lodash": ">=3.5 <5",
"mutexify": "1.0.1"
"mutexify": "1.0.1",
"toposort": "^1.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move that to devDependencies

Copy link
Contributor

@mastilver mastilver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small details and good to go :)

vendor: './util.js'
},
output: {
filename: '[name].out.js'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you set output.path to path.join(__dirname, 'build') and filename to [name].js

I've got plan for the future to use those in tests, that will make it easier

var nodeMap = {};

files.forEach(function (file) {
nodeMap[file.chunk.id] = file;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an isChunk guard.

var edges = [];

files.forEach(function (file) {
if (file.chunk.parents) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i'm not actually all that familiar with webpack, what kind of dependency tree would give me files without chunks? I supposed the example should handle that case? or is it sufficient to handle the actual case that the example files dep tree creates (all files are chunks?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops sorry, missed that comment.

We include emitted files (for example from copy-webpack-plugin), basically it's every non-js files. I think those better be at the end (but really I don't care, up to you!)

@SeeThruHead
Copy link
Contributor Author

i can take a look at this tmrw. what better way to spend my airport wait :D

@mastilver
Copy link
Contributor

Hey @SeeThruHead :)

Let me know how you are getting on with that. Do you want me to take it over (I will have more time in a couple of weeks)?

@SeeThruHead
Copy link
Contributor Author

hey @mastilver I made a comment above asking for some guidance with how to test an isChunk guard. ie what kind of dep tree would give me files without chunks.

@akrawchyk
Copy link

This is really good stuff, I'd love to see it get merged so it gets more visibility!

One thing I noticed in the example is there is a manifest.concat() call, and according to the current docs that should be an Object not an Array so this example will fail.

It's my first time trying this plugin, so maybe that is for a different version? Wondering if this PR needs some TLC before it gets merged.

@caub
Copy link

caub commented Jun 20, 2018

it's a bit brutal, but I've done it here https://github.com/caub/webpack-manifest-plugin/blob/m2/lib/plugin.js (the logic actually mostly copied from HtmlWebpackPlugin)

I think the manifest file deserves to be sorted by default, I couldn't use the generate option easily

@mastilver
Copy link
Contributor

mastilver commented Jun 25, 2018

html-webpack-plugin is getting rid of toposort in flavours of entrypoints, we should do the same for this plugin I believe

@caub
Copy link

caub commented Jun 25, 2018

I think it's possible to get rid of it only with webpack4, but not sure

@shellscape
Copy link
Owner

@SeeThruHead Thanks for opening this PR a while back, and I'm sorry that it didn't get attention sooner. We've just landed a major refactor in #222, and part of that is only supporting webpack v4.44.0+. Please let me know if you're interested in reopening this PR and we'll move forward.

@shellscape shellscape closed this Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants