search

browserify / factor-bundle

factor browser-pack bundles into common shared bundles
Other
402 stars 27 forks source link

threshold function #8

Closed edrex closed 10 years ago

edrex commented 10 years ago

implements #6

edrex commented 10 years ago

This is unsafe, since the threshold function can put a file in common but not its dependency, resulting in runtime module not found exception.

@substack, should this feature be made safe from the above issue?

ghost commented 10 years ago

Yes.

edrex commented 10 years ago

To make it safe, we will have to short-circuit the common threshold to true if any of the file's dependents are in common. Looking at how that might be done.

edrex commented 10 years ago

I took a shot at fixing the issue with dependees being below the common threshold when their dependents are above in 6c9f84d3ce2987ff610c26e4ba5e6b01ef92ca72 (outside this PR). It works partially, but (see included test) fails when a dependee is also present lower in the tree and happens to come before the dependent.

To make this work self._buffered would need to be topologically sorted.

Seems to be getting too complicated. Ideas? Maybe trust the threshold function to grab dependees?

edrex commented 10 years ago

I think making the output of module-deps topologically sorted would allow this to work.

ghost commented 10 years ago

https://npmjs.org/package/deps-sort

edrex commented 10 years ago

@substack that doesn't seem to do topological sort, just sorts by id (path).

Seems like having module-deps output topo sorted by default might be a nice property.

edrex commented 10 years ago

I did the sort locally which makes the test pass so I guess this is ready for review.

Maybe the sort should be pulled out into a deps-toposort module.

ghost commented 10 years ago

Looks good. Merged in 0.0.2.

ghost commented 10 years ago

oh whoops that should have been a minor version bump but too late now