Create implied dependency on dedupe target when sorting

[jgoz]

As discussed in #38.

This ensures that dedupe targets will be processed after dependencies that point to them.

I restructured the test case to fake this scenario and I'm open to suggestions on better ways to solve this. This solution has the advantage of working with the existing deps-topo-sort (and it cleans up after itself immediately), but it's kind of hacky.

[terinjokes]

What would be the alternative changes to deps-topo-sort? Seems like it could be easier than the hacking going on here.

[jgoz]

Well, there are a few options:

1. Make deps-topo-sort aware of dedupe and dedupeIndex, treating those as alternate dep sources for a row. I'm not crazy about this because it introduces browserify-specific logic into that module.
2. Create the implicit dependencies in browserify directly or by hooking into to the dedupe pipeline in factor-bundle. I'm not crazy about either of these options because those dependencies are not real and it might introduce weird side effects.
3. Add something like an implicitDeps array that deps-topo-sort is aware of and that gets created in a dedupe pipeline hook in factor-bundle.

I like 3 the best, but we'd still be adding extra functionality to deps-topo-sort solely for this plugin.

[terinjokes]

I agree, it does seem strange to introduce browserify-specific logic into deps-topo-sort. I was briefly thinking they came from module-deps, but upon review I see that I'm wrong.

I dislike two for the same reason I dislike the small amount I'm already reaching into browserify internals, and agree that putting deps where there aren't any could cause problems elsewhere.

On review, and on comparison, what you've done so far is good. I think I'd prefer moving the wrapping of deps-topo-sort somewhere not in the middle of createStream, though.

[jgoz]

I dislike the small amount I'm already reaching into browserify internals

I know what you mean. Your v2 reduced the dependency on internals by a lot, but it's still not a "pure" plugin.

I'd prefer moving the wrapping of deps-topo-sort somewhere not in the middle of createStream, though.

Sure, I'll refactor.

[jgoz]

@terinjokes Moved sort stream creation to a function at the bottom. Alternative was creating the stream directly at the top somewhere.

[jgoz]

After some more thought, solving this in browserify might actually be the right way to do it.

When browserify sees an exact match during dedupe, it replaces the source of the duplicate source file with a require statement that points to the original row's index and clears the dup's deps object. You could argue that this paints an inaccurate picture of the dependency graph because the duplicate row does now depend on the original row.

The solution would be to set row.deps[row.dedupe] = row.dedupeIndex after clearing it during dedupe. That way, the dependency that has been created by browserify is recorded and downstream plugins (like factor-bundle) can use that information properly.

@substack, @terinjokes What do you think?

[jgoz]

@terinjokes @substack https://github.com/substack/node-browserify/pull/880?

[terinjokes]

Thanks.

[ghost]

Merged in 2.2.0. Thanks for the patch!

[jgoz]

@substack Sorry, I meant to close this before substack/node-browserify#880 got merged - that PR made this one unnecessary.

I think this change should be reverted. I can submit a request for that later tonight if you like.