Build client-side Mocha using Browserify #1727

ndhoule · 2015-06-06T22:11:33Z

Still needs some work before merging but wanted to put it out there early since it seems like a better build process is eating up time and making stuff like the plugin API and breaking reporters out into npm-land difficult.

Outstanding issues:

Still some cleanup to do, current transition is a little messy. And I bet we can remove a few more shims like lib/browser/events.
Did super cursory testing in the browser and even more cursory at the command line; would want to stress test this a bit more before merging.
The built Browserify file is still a lot larger than the current build process (~148k currently, 209k under Browserify). Haven't looked into this too deeply yet.
Browserified file includes all the reporters, which @tj said shouldn't happen anyway. I think, though, that removing them would break mocha-phantomjs; @metaskills, is that right? Is that something to be concerned about?

Will follow up on these in the next day or so, when I get a bit more time.

Related issues:

rename browser require() implementation #1193
build mocha.js using browserify #1260
fix(utils): issue #1558 + make #1559
browserify breaks at require('./lib-cov/mocha') #880 (this PR skips Jade in Browserify via --ignore jade, which I think was causing @jonathanong's issue back in build mocha.js using browserify #1260)

Tangentially related:

🚀 Feature: Plugin API #1457

ndhoule · 2015-06-06T22:21:09Z

lib/mocha.js

@@ -143,7 +144,9 @@ Mocha.prototype.reporter = function(reporter, reporterOptions){
  } else {
    reporter = reporter || 'spec';
    var _reporter;
-    try { _reporter = require('./reporters/' + reporter); } catch (err) {}
+    // Try to load a built-in reporter.
+    if (reporters[reporter]) _reporter = reporters[reporter];


Worth pointing out here is that Browserify doesn't do dynamic requires, so we have to switch to a static approach here. I assumed that reporter is always in lowercase, not sure if that's a safe assumption or if we want to implement a more robust approach to grabbing built-in reporters here (spitballing here: e.g. always normalize to kebab-case)?

dunno. I wanted to eliminate them from the core except for spec, so whatever's clever for now.

Couldn't agree more with @boneskull.

👍 works for me.

jbnicolai · 2015-06-06T23:34:21Z

Note that the failing TravisCI build is only on node v0.8 due to the caret-notation used on the browserify dependency, something older npm versions can't handle.

jbnicolai · 2015-06-06T23:36:18Z

Makefile

@@ -1,4 +1,5 @@

+BROWSERIFY := node_modules/.bin/browserify


I guess we could do something like PATH := node_modules/.bin:$(PATH) to simplify the code (slightly). Seeing as you'd also prefer a local dep. over a global one anyway.

👎 Makefiles

@boneskull why? To me this still seems more straightforward than most alternatives (which admittedly says a lot about the alternatives as well..)

Yeah, I prefer local dependencies. Gives you everything you need in an npm install and prevents "what version of Browserify are you running, oh, update/downgrade it"-type issues.

I generally just throw all my binaries at the top of the file because I've has issues with the PATH solution in the past (can't remember what those were), I'm open to whatever though

Nah, the number of paths explicitly stated is small enough that it doesn't matter, although I'm curious what your issues were.

No clue, I can repro it in my other local repos, but not in my local Mocha repo. Weird.

I also prefer the explicit path just because I keep global installations of some binaries (e.g. ESLint) installed for editor integrations, etc. and like to be sure there's nothing funky going on, like a missing local dependency triggering errors because it's falling back on a global installation, which is a heinous thing to debug. More verbose for fewer terrible things to debug, but maybe that's crossed over into paranoia territory.

Anyway, it's working locally so I don't mind either way

boneskull · 2015-06-06T23:40:26Z

let's just keep the browserify dependency exact. given that you can't build the browser version of Mocha w/o it, I don't want to take any chances

jbnicolai · 2015-06-06T23:40:58Z

lib/reporters/base.js

-  : false;
+exports.useColors = process.browser
+  ? false
+  : (supportsColor || (process.env.MOCHA_COLORS !== undefined));


A ternary where one of the statements is a boolean constant always seems odd to me. How about

!process.browser && (supportsColor || process.env.MOCHA_COLORS);

I'm in agreement here fwiw

Ah yeah, I just kept it that way because process.browser is undefined in Node, but I'll stop being lazy

boneskull · 2015-06-06T23:41:17Z

Browserified file includes all the reporters, which @tj said shouldn't happen anyway. I think, though, that removing them would break mocha-phantomjs; @metaskills, is that right? Is that something to be concerned about?

there's only one reporter in the browser...

boneskull · 2015-06-06T23:41:30Z

The built Browserify file is still a lot larger than the current build process (~148k currently, 209k under Browserify). Haven't looked into this too deeply yet.

I don't care about this at all

boneskull · 2015-06-06T23:41:53Z

Still some cleanup to do, current transition is a little messy. And I bet we can remove a few more shims like lib/browser/events

yes please. thanks for the tremendous amount of work here.

ndhoule · 2015-06-06T23:45:08Z

@boneskull

there's only one reporter in the browser...

https://github.com/metaskills/mocha-phantomjs#standard-out
I think mocha-phantomjs relies on all the reporters being present in the browser (which is why Mocha.process is exposed).

jbnicolai · 2015-06-06T23:49:32Z

thanks for the tremendous amount of work here

yes!

I don't care about this at all

Same. Make it work first, optimize later. ;)

I think, though, that removing them would break mocha-phantomjs; @metaskills, is that right? Is that something to be concerned about?

I wouldn't worry about this. The other reporters should be broken-out and included separately and explicitly by mocha-phantomjs

ndhoule · 2015-06-06T23:49:44Z

Bleh, looks like Browserify's deps have carets in them so they hose Node 0.8. Could just add a Make target to install it before building mocha.js I guess, and note in the docs that Mocha works, but is unbuildable on, 0.8. Hacky, but would work—objections?

jbnicolai · 2015-06-06T23:51:46Z

Yes.. the issue is not even node 0.8 but the bundled npm version. Any production environment stuck due to whatever complexities on an older version would still easily be able to update npm, so I wouldn't consider it an issue at all.

We could add a line to the .travis.yml like:

before_install:
- npm install -g npm@latest

jbnicolai · 2015-06-07T00:06:51Z

The built Browserify file is still a lot larger than the current build process (~148k currently, 209k under Browserify). Haven't looked into this too deeply yet.

I don't care about this at all

If we'd like we could get this down by about 3x using Google's closure compiler.

make
for f in mocha{,.browserified}; do 
  curl --data-urlencode "js_code@$f.js" \ 
    -d "output_format=text&output_info=compiled_code&compilation_level=SIMPLE_OPTIMIZATIONS&language=ECMASCRIPT5" \ 
    http://closure-compiler.appspot.com/compile > "$f.min.js"; 
done
du -hs | gshort -h

88K mocha.browserified.min.js
88K mocha.min.js
208K mocha.browserified.js
208K mocha.js

boneskull · 2015-06-07T00:09:00Z

I wouldn't worry about this. The other reporters should be broken-out and included separately and explicitly by mocha-phantomjs

I don't think karma-mocha is reliant on the reporters either. It has its own. I'm not sure where it hooks in, though.

boneskull · 2015-06-07T00:09:36Z

I don't think karma-mocha is reliant on the reporters either. It has its own. I'm not sure where it hooks in, though.

(you can use phantomjs as the browser)

jbnicolai · 2015-06-07T00:12:23Z

Played with it a bit more, I'm all for this PR 👍

ndhoule · 2015-06-07T00:14:10Z

@jbnicolai

Yes.. the issue is not even node 0.8 but the bundled npm version. Any production environment stuck due to whatever complexities on an older version would still easily be able to update npm, so I wouldn't consider it an issue at all.

We could add a line to the .travis.yml like:

Haha good point, not sure why I didn't think about that. Changed

jbnicolai · 2015-06-07T00:26:33Z

support/rewriteInheritance.js

+ * @return {string}
+ */
+function parseInheritance(js) {
+  return js.replace(/^ *(\w+)\.prototype\.__proto__ * = *(\w+)\.prototype *;?/gm, function(_, child, parent){


:/ I appreciate the effort, but we really should remove all usages of __proto_ from the codebase. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto for instance.

Couldn't we apply this rewrite rule verbatim to our codebase, rather than at build-time?

git grep __proto lib

lib/hook.js:Hook.prototype.proto = Runnable.prototype;
lib/reporters/dot.js:Dot.prototype.proto = Base.prototype;
lib/reporters/landing.js:Landing.prototype.proto = Base.prototype;
lib/reporters/list.js:List.prototype.proto = Base.prototype;
lib/reporters/min.js:Min.prototype.proto = Base.prototype;
lib/reporters/nyan.js:NyanCat.prototype.proto = Base.prototype;
lib/reporters/progress.js:Progress.prototype.proto = Base.prototype;
lib/reporters/spec.js:Spec.prototype.proto = Base.prototype;
lib/reporters/xunit.js:XUnit.prototype.proto = Base.prototype;
lib/runnable.js:Runnable.prototype.proto = EventEmitter.prototype;
lib/runner.js:Runner.prototype.proto = EventEmitter.prototype;
lib/suite.js:Suite.prototype.proto = EventEmitter.prototype;
lib/test.js:Test.prototype.proto = Runnable.prototype;

It looks like the transform below would change

Hook.prototype.__proto__ = Runnable.prototype;

into

function F(){}; F.prototype = Runnable.prototype; Hook.prototype = new F; Hook.prototype.constructor = Hook;

Is there any reason to not just apply this to the entire codebase and remove this rewrite rule?

Haha, no real effort involved, I just ported it from here:

https://github.com/mochajs/mocha/blob/master/support/compile.js#L59

But yeah, we should definitely fix it throughout. I just figured we could handle that as a separate issue and keep this PR's scope strictly to switching to Browserify, since it already runs the risk of breaking so many things

Completely forgot about that, thought you dug it up somewhere else ;) (heh, my name is even in the edit-list of said file.. oops).

Let's leave this for now :) although a future improvement would definitely be getting rid of these.

errr... yeah. we need to get rid of all of these hinky shims and use something well-supported. if we're targeting environments that don't support Object.create() then we should just do something like use lodash.create

ndhoule · 2015-06-07T00:53:07Z

Is there an established/easy way to do cross-browser testing against the built lib right now? I can just do some informal Sauce tests if not

jbnicolai · 2015-06-07T01:06:29Z

@ndhoule there's not. It might be about time to add some selenium based tests.

boneskull · 2015-06-07T04:37:06Z

@jbnicolai go for it dude, you're on a roll 😉

ndhoule · 2015-06-07T19:55:58Z

Had a chance to do some IE < 9 browser testing; glad I did, exposed a few breakages. Fixed and rebased. I think this is about ready to merge.

Adding CI would be a good thing because it's easy to introduce an ES3-incompatible shim with Browserify. Started an issue over here: #1732

Going to punt on the reporter removal (#1731, feel free to close as a dupe/consolidate issues/etc.) just because I'm concerned it'll break mocha-phantomjs, thinking we should give them some heads up / provide a solution before we remove it. Make sense?

nathanboktae · 2015-06-08T17:31:32Z

Yes removing the reporters will break mocha-phantomjs and mocha-casperjs - both use the compiled mocha.js and change the reporter as needed. I'll provide more details in #1731

ndhoule · 2015-06-18T18:47:42Z

Yeah, open a separate issue. This might solve it but they're not the same issue, so better to track them separately

FWIW you can probably just wrap Mocha and your built stuff in an IIFE and concat the files together.

wejrowski · 2015-06-18T18:58:25Z

Using browserify? How would the IIFE solve that? Not totally tracking.

And thanks. I will play with it some more and then open a ticket. Would love to solve this. I'm using Jasmine right now, which is great because it is just one file, but I'm having test issues with it.

ndhoule · 2015-06-18T19:04:26Z

@wejrowski basically something like:

// head.js
(function() {

// tail.js
}).call(this);

Then just glue these fragments, mocha, and your browserified bundle together:

cat head.js mocha.js browserified-stuff.js tail.js > build.js

Or something. You'll probably have to play with it a bit to get it working right.

I'm using Jasmine right now, which is great because it is just one file

Isn't mocha just one file too, though? Not totally following

jbnicolai · 2015-07-04T20:51:38Z

@ndhoule apologies for the delay, I'm finally back home and ready to put some time in mocha again ;)

It seems the (fantastic!) #1750 has now been merged. Could you rebase this PR to reflect those changes?

jbnicolai · 2015-07-04T20:52:18Z

@wejendorp the built mocha file is just single dependency-free file.. please open a separate issue if you'd like to discuss this further.

ndhoule · 2015-07-05T01:30:58Z

@jbnicolai Rebased.

ndhoule · 2015-07-05T17:08:26Z

Removed the __proto__ transform, since #1753 makes it unnecessary

jbnicolai · 2015-07-05T17:09:15Z

Ah, was just doing this locally as well. Thanks 👍

jbnicolai · 2015-07-05T17:14:20Z

Other than that I think it's good to merge :)

jbnicolai · 2015-07-05T17:14:38Z

Oh, could you squash that last commit?

ndhoule · 2015-07-05T17:16:25Z

Oh, could you squash that last commit?

Done

ndhoule · 2015-07-05T17:26:40Z

Awesome, glad to see it go in 🚢 🚢 🚢

Thanks!

boneskull · 2015-07-05T17:32:41Z

@ndhoule @jbnicolai awesome! thanks much!

jbnicolai · 2015-07-05T17:56:58Z

All credits go to @ndhoule, thanks for the (major!) effort, and we'll be sure to mention it in the release notes :)

Great step towards making mocha a more modern and maintainable project!

boneskull · 2015-07-05T18:03:37Z

@jbnicolai until someone makes a PR building mocha with jspm...

ndhoule reviewed Jun 6, 2015
View reviewed changes

jbnicolai reviewed Jun 6, 2015
View reviewed changes

jbnicolai reviewed Jun 7, 2015
View reviewed changes

ndhoule mentioned this pull request Jun 7, 2015

Replace __proto__ throughout with setPrototypeOf shim #1728

Closed

ndhoule force-pushed the chore/browserify branch 2 times, most recently from 6f4051d to 2f11173 Compare June 7, 2015 19:47

ndhoule mentioned this pull request Jun 7, 2015

Remove non-HTML reporters from built mocha.js #1731

Closed

ndhoule force-pushed the chore/browserify branch 3 times, most recently from 2d24882 to d3b0305 Compare July 5, 2015 01:28

jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25

ndhoule force-pushed the chore/browserify branch from d3b0305 to 832e917 Compare July 5, 2015 17:06

Build using Browserify

11e43e3

ndhoule force-pushed the chore/browserify branch from cf25e44 to 11e43e3 Compare July 5, 2015 17:15

jbnicolai closed this in 39ce95f Jul 5, 2015

ndhoule deleted the chore/browserify branch July 5, 2015 17:26

ndhoule mentioned this pull request Jul 5, 2015

🚀 Feature: Use process.browser to detect environment #1764

Open

marekventur mentioned this pull request Dec 14, 2015

Update "mocha" to version ~2.3.4 gluckgames/pixi-packer#17

Merged

ndhoule mentioned this pull request May 16, 2016

Fix missing methods/globals in IE7, IE8 environments #2259

Closed

snyk-bot mentioned this pull request Nov 18, 2020

[Snyk] Fix for 1 vulnerabilities qsays/mocha#10

Open

Build client-side Mocha using Browserify #1727

Build client-side Mocha using Browserify #1727

Conversation

ndhoule commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbnicolai commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Jun 6, 2015

boneskull commented Jun 6, 2015

boneskull commented Jun 6, 2015

ndhoule commented Jun 6, 2015

jbnicolai commented Jun 6, 2015

ndhoule commented Jun 6, 2015

jbnicolai commented Jun 6, 2015

jbnicolai commented Jun 7, 2015

boneskull commented Jun 7, 2015

boneskull commented Jun 7, 2015

jbnicolai commented Jun 7, 2015

ndhoule commented Jun 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhoule commented Jun 7, 2015

jbnicolai commented Jun 7, 2015

boneskull commented Jun 7, 2015

ndhoule commented Jun 7, 2015

nathanboktae commented Jun 8, 2015

ndhoule commented Jun 18, 2015

wejrowski commented Jun 18, 2015

ndhoule commented Jun 18, 2015

jbnicolai commented Jul 4, 2015

jbnicolai commented Jul 4, 2015

ndhoule commented Jul 5, 2015

ndhoule commented Jul 5, 2015

jbnicolai commented Jul 5, 2015

jbnicolai commented Jul 5, 2015

jbnicolai commented Jul 5, 2015

ndhoule commented Jul 5, 2015

ndhoule commented Jul 5, 2015

boneskull commented Jul 5, 2015

jbnicolai commented Jul 5, 2015

boneskull commented Jul 5, 2015