Order of files added to bundle is non-deterministic

[smrq]

Browserify respects the order of files that are included, and sometimes that ordering can be significant. I have a situation in one of my codebases in which tests sometimes fail because b.add() is called on the input files in a different order.

I'm pretty sure this is because Karma runs testFilePreprocessor in a non-deterministic order on the input files. I think that Karma normally includes files in the order specified in files in its config, but that doesn't mean much here since the individual files are replaced with stubs. (Correctly ordered stubs!) Can, and should, karma-bro respect that ordering for files it adds to its bundle?

[bendrucker]

Any chance you could figure out a failing test for this? Not sure if it's possible to change this behavior:

https://github.com/Nikku/karma-bro/blob/master/lib/bro.js#L198-L200

[smrq]

I can probably make a failing test. I can definitely make a test case that fails some of the time 😆

That's probably a good place to do it, too. I was thinking of trying to control the order of the internal files array, but that would be pretty annoying to implement!

[smrq]

Ok, I updated preprocess() in the tests to act a bit more like Karma, and strengthened some assertions so that they now fail. The changes are on smrq/karma-bro#bundle-ordering. I will probably try and resolve #34 while I think about the best solution here.

[smrq]

I just had sort of a weird idea for this that would guarantee the correct order of execution. What if, instead of replacing each file with a shim like /* bundled */, each file was added to the bundle with b.require({ expose: fileName }) and each shim replaced with require(fileName)? In a way I feel like that makes the entire bundle inclusion less hacky overall, and it has the benefit of guaranteeing that the order of execution will be the same as what Karma does without bundling.

There is one critical problem with this plan at the moment: browserify/browserify#937. If there aren't any entry point files (i.e. files added to the bundle with b.add() or b.require({ entry: true })) then transforms won't run. I think a workaround until that issue is fixed would be to add essentially an empty stub file as an entry point.

This is all in theory but I think it should work. I'll try and implement it and see how the code looks.

[smrq]

I did an implementation of the above, and it passes all of the tests... except on Windows where everything blows up catastrophically for mysterious reasons I'm still working on. I do think it's a worthwhile approach.

[nikku]

this was the original implementation of karma-bro in browserify 4x times.

test cases are most important here because I'd otherwise would remove/refactor that feature a some point ;)

[smrq]

Figured out the Windows problem, it's this: browserify/browserify/pull/946 With that change (and some minor updates to expecteds) all tests pass on Windows as well.

@nikku Aw man, so I'm reinventing a wheel here? Well, if we both came up with it independently then it must be a good idea, right? 😆 What was the reason for changing the implementation-- Browserify 5.x breaking it, code cleanup, something else? I don't want to reintroduce any problems that may have been solved in the past.

As for test cases, what I've got right now in my branch definitely won't pass if the implementation was changed back. It can't be totally bulletproof since the karma-bro tests don't run a real instance of Karma, and the ordering is defined by how Karma includes the test stubs.

[bendrucker]

Awesome work @smrq!

See https://github.com/Nikku/karma-bro/blob/v0.6.0/lib/bro.js#L233 for the old implementation. Pretty sure the behavior where an entry point is required for transforms to run was new to browserify@5. If need be, it would be okay to actually spin up a karma instance and run actual code as "integration tests." Not sure that's necessarily the best idea since it would be pretty convoluted to get real assertions written, let alone understand them if one you didn't write was failing. If you can think of a way to make it happen it's worth a look.

[nikku]

I'd like to switch back to your (@smrq) áka my old implementation. Just makes test cases much more robust.

@smrq We are waiting for browserify/browserify#948 to be merged, right?

[nikku]

Would be a good candidate for 1.0.0.

[smrq]

@nikku Yup, once that PR gets merged then what's in my fork should be solid. I'm cool with the tests as-is... as @bendrucker says, spinning up a real Karma instance is likely more trouble than it's worth.

[nikku]

Let us just stick to simple test cases + a bit of manual integration testing then.

[bendrucker]

I'd actually suggest getting this out as 0.10 so we can get a few people using it before making the big 1.0 switches.

[smrq]

The relevant PR just got merged into Browserify. I'll have a PR for this up tomorrow!

[bendrucker]

Lovely! Speaking of which, it would be super cool to have a little bot for subscribing to remote issue changes and posting them to the original issue thread. Yet another side project to tackle one day...

[bendrucker]

Fixes merged in d58a272