add new mocha to zuul

[phated]

I noticed #219 went stale. Also, mocha has recently switched to using browserify to build. However, I looked into programmatically generating the build as part of zuul, and it wouldn't work very cleanly because mocha doesn't ship their support/browser-entry.js file in their published version. This means that zuul would have to ship a copy of that file, which seems worse than just shipping the framework because a change in the lib or browser-entry file could break zuul completely.

I've also built the latest browser-compatible file from source because they had a bug in the latest committed build that broke the runner.

If you don't want to use the browserified version, let me know, but I think that this gets zuul closer to building on the fly.

Closes #219
Closes #201

[defunctzombie]

The new mocha framework file is much larger. What is that due to? Possibly inclusion of all the various bells and whistles like reporting that we don't need? Can we cut that down?

Do you think it is better to include mocha in-tree or have users install it in their own repos? That way they can upgrade mocha as they please. I could see pros/cons for each. Probably easier in-tree for now but could be convinced otherwise.

[phated]

I think that having people install it would be ideal, but I don't think that is possible because mocha in-browser doesn't work without being built with their special parts (support/browser-entry.js and all files in lib/browser/). Given that, I think it should stay in-tree for now.

I am noticing the reporters in the bundle, which can probably be ignored. Let me try to pull them out.

[phated]

@defunctzombie digging through the built file, it seems that the reporters aren't adding that much size. Most of the extra size is being introduced because process.stdout is used and they bring in the process-stdout module to shim it, which requires stream. Also, they rely on Buffer in a few modules (including streams once included) which adds a ton of weight to the bundle.

Thoughts?

[defunctzombie]

Sigh. Why do they have so much going on in what should be a relatively simple thing...

Do they make use of the browser field in package.json or some other way we could re-shim process.stdout to remove this. I can't imagine all this bloat is actually useful in browser. My main concern with larger file size is when users test in cloud and it will take longer to download.

What is the file size delta from these changes?

ping @jbnicolai @danielstjules @travisjeffery who all appear to be current/recent Mocha contributors.

[phated]

The file size goes from 119K to 331K unminified/unzipped on my computer, which ends up being 27.9kB vs 77.49kB when gzipped.

The mocha package.json uses the browser field that points at debug, events and tty - https://github.com/mochajs/mocha/blob/master/package.json#L305-L309 - however, I think the dependencies-dependencies are still bringing in modules like node's EventEmitter.

I am currently exploring removing the reporters, which should remove the need to tty, which should allow me to remove the process-stdout module

[phated]

I had to do a bunch of zuul-specific changes, but I've gotten the size down significantly. I'm going to fork mocha and submit the new built version here to see what you think.

[phated]

Alright, so the work to make a smaller zuul-friendly bundle was done in phated/mocha@a184f87 - I'm pretty sure mocha won't accept those changes upsteam. If zuul continues with the in-tree mocha, I think it should be fine to maintain this fork just for building. Thoughts?

[phated]

I've just pushed the newly built version, based off the linked fork. The file size (gzip) comparison is now 27.9kB (original) vs 37.68 kB (updated)

[phated]

Almost all of that is due to their dependency on the buffer module.

[defunctzombie]

I like it! Lets see what the mocha devs say about shipping a smaller browser side bundle capability in mocha. If not, we can just work off what you have in our tree and make a documentation note about this so we re-apply your patch when we rebuild mocha in the future.

[defunctzombie]

@phated if we don't hear from them in a day or two then I will just merge this in and we will use your fork for mocha.

[danielstjules]

So ideally, zuul would require a build of mocha without the reporters?

[defunctzombie]

@danielstjules yea. Basically, the "browser" build of mocha needs to be more minimal for the "core" features (like running and reporter API) and then if users want extra reporters, etc they could require those separately if that makes sense.

[phated]

Mocha fails to initialize if no reporters exist, so I had to stub them. Also, the browser-stdout shim needs to be removed.

[danielstjules]

@defunctzombie The plugin API would solve this, but that's not ready for a release. And a quick extraction of the reporters would mean rushing out a new major since it would be a breaking change. If this is something you'd want ASAP, I don't think there's much we could do to help.

If possible, I'd suggest using mocha 2.2.4 or 2.2.5 (latest). Neither use browserify. It's 145KB vs 119KB (current) vs 171KB (the forked version).

If you do intend to fork, maybe use a tagged release, rather than what's in master? It would hopefully be easier to manage upstream changes.

The only other immediate alternative would be for the mochajs org to host the fork, shifting the burden of who maintains the build.

Now that you've highlighted how bloated the build is in master, we'll hopefully get a fix out before the next minor release. :)

[danielstjules]

Btw @phated thanks for the help with this!

[defunctzombie]

@phated which version of mocha did you try to bundle? What are your thoughts on the plugin API and waiting for next release?

[danielstjules]

which version of mocha did you try to bundle?

https://github.com/phated/mocha/commits/master is currently up-to-date with mochajs/mocha master: That's 71 commits ahead of the last release

[danielstjules]

Based on the initial post in this PR, were there issues with 2.2.5 or 2.2.4 preventing you from using them?

[defunctzombie]

@danielstjules a bit confused at the recommendation on using mocha 2.2.4/5 then? Are you saying we should not have tried to use master but instead used the released version?

[danielstjules]

I think I misunderstood the initial request, sorry! If you specifically require mocha to be built with browserify, I'd probably suggest waiting until the next minor release if you'd like to avoid a fork. We should have things cleaned up a bit by then. :) But if you're just hoping to use a newer version of mocha, I'd suggest using 2.2.x

[phated]

I used master to get the browserified version because it has often been talked about in zuul threads I've looked at. I would be fine changing this PR to the latest tagged release, as I am just hoping to get a new version in zuul. However, I am glad I took the "build from master" route first so we could find these things before mocha shipped the standard browserify build. @defunctzombie thoughts?

[phated]

@defunctzombie what should I do to wrap this up? I really just want a newer version of mocha, so I am open to shipping the non-browserify version from the current stable release.

[defunctzombie]

Does current stable work for you? If it does the. Yea lets do that and we
can revisit packaging if/when a new release lands.

On Monday, August 17, 2015, Blaine Bublitz notifications@github.com wrote:

@defunctzombie https://github.com/defunctzombie what should I do to
wrap this up? I really just want a newer version of mocha, so I am open to
shipping the non-browserify version from the current stable release.

—
Reply to this email directly or view it on GitHub
#227 (comment).

[defunctzombie]

@phated does this require a new PR or is this one good to go?

[phated]

I'll be updating this PR before the end of the day. I'll ping when updated

[phated]

@defunctzombie I've updated this PR to be the latest stable release of mocha instead of master. It is working for me against my linked project.

[defunctzombie]

@phated does the new mocha work in older IE or at least same IE range as before?

[phated]

@defunctzombie it looks like it by the History.md file (a few versions back they fixed an IE8 bug - see mochajs/mocha#1617). Does the zuul CI test on old IE? I know sauce is a pain to get tested against a PR, but maybe you can merge this into a branch and then push to zuul to get them to run?

[defunctzombie]

I am running some local tests now.