search

browserify / factor-bundle

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

Handle deduped rows #38

Closed jgoz closed 10 years ago

jgoz commented 10 years ago

If a deduped row is factored into the common bundle, the target row should also become common.

This can cause an issue when the same dependency can be resolved to multiple paths, but only one of them becomes common.

A concrete example would be with using assert and util in your web project. The assert module is pinned to an outdated version of util, whereas browserify uses the latest util, resulting in assert/util and util being extracted to node_modules. Code that uses assert will transitively use the outdated version of util, which browserify can dedupe to its actual util. But if assert/util is used multiple times when util is only used once, a runtime error is thrown because the assert/util dependency on util is not available in the common script.

This fixes that :)

jgoz commented 10 years ago

@substack, @terinjokes I'm only checking for row.dedupeIndex here rather than that and row.dedupe, because browserify only looks at dedupeIndex. Thoughts on that choice?

ghost commented 10 years ago

This seems like a fine choice until we get some failing tests indicating otherwise. Merged in 2.1.3. Thanks for the patch!

terinjokes commented 10 years ago

Only thing I can think of is that deps-topo-sort only looks at the deps field so it's possible that other module had already been written out to a stream. Trying to come up with a failing case to see if it's possible.

terinjokes commented 10 years ago

I shuffled around the dependencies and indexes, and wasn't able to come up with anything failing. Will keep eyes peeled for a failing report.

:+1:

jgoz commented 10 years ago

Thanks @terinjokes. I'll try to come up with a test case for that scenario because I think it's valid (though probably rare). You might be able to handle it by adding transform streams before and after deps-topo-sort that insert (and later remove) a fake dep entry for deduped rows.

terinjokes commented 10 years ago

I ended up just faking it just by editing the orders of the ROWS. Can probably push a commit that makes it part of the test suite.

jgoz commented 10 years ago

K, I think we should handle it - I'll give you an empty branch for your commit, then I'll do the fix.

jgoz commented 10 years ago

Actually, you can just push it to support-dedupe on my fork. You should be a collaborator.