audit utils.js

[boneskull]

There may be some code in utils.js that would be better handled by 3p modules, or fully eliminated.

Depending where you stand, 3p deps may be good or bad.

An argument "for" would be that we don't have to maintain code. An argument "against" is that we have to live with the decisions of whoever maintains the code.

Identify functionality that may be replaced by (better) 3p modules, and raise issues for each.

[jbnicolai]

Depending where you stand, 3p deps may be good or bad.

I'd be interested in your opinion on this. Personally, as long as the dependencies are stable and proven, I'm all for it as it relives Mocha of bloat and the burden of having to maintain that module.

It seems the IE8 compatability functions can be replaced by es5-shim.

escapeRegexp could be replaced by escape-string-regexp. It seems this function also escapes the character -, which has no reason as far as I can tell.

I large part of reporters/base.js could probably be removed by using a CLI colouring module like chalk.

[boneskull]

I'd be interested in your opinion on this. Personally, as long as the dependencies are stable and proven, I'm all for it as it relives Mocha of bloat and the burden of having to maintain that module.

Agreed, 100%.

• es5-shim: 👍
• escape-string-regexp: 👍

I haven't used chalk but I've heard it's very good. However, I'd probably want to sit on the reporter stuff if we're going to try and move forward with the js-reporters initiative.

[jbnicolai]

Implemented escape-string-regexp, but decided against es5-shim upon second thought.

Mocha is a dependant of many other packages and projects, and it's bad form to modify globals without explicit warning.

We could still decide to replace these functions with, for instance, underscore though?

[jbnicolai]

Excited about the js-reporters initiative by the way. Haven't found the time yet to get involved, but definitely a 👍

[boneskull]

👎 on underscore given it's track record.

LoDash if anything, but that seems like a rather large dependency. Still, that's a very well-maintained library. I'll need to take a gander @ our code and understand the current state of the shims.

[jbnicolai]

Well, that was an interesting read. What was the maintainer of underscore thinking? o.O

I agree that it's not worth introducing something as large as LoDash.

I'll need to take a gander @ our code and understand the current state of the shims.

It's only a few, and they are only used in a couple of places. I guess we'll leave it as is.

[travisjeffery]

ya lodash if anything but for changed like this - make them as you need to or maybe as you're fixing something related. i.e. if something's working don't change it for the sake of changing it.

[boneskull]

@travisjeffery Sounds fair. The purpose of course is to reduce LOC, but no point in tinkering w/ it until we have to touch those lines.

I'll leave this open so we can make notes as we run across things.

[jbnicolai]

👍

[rlidwka]

Well, that was an interesting read. What was the maintainer of underscore thinking? o.O

I believe they were thinking exactly the same thing as you in #1228

Just depend on it with ~ instead of ^, that's all; and I'd really prefer that over lodash. But in node.js it's easy enough to avoid any of them using native methods.

[jbnicolai]

I believe they were thinking exactly the same thing as you in #1228

Fair point. Although in retrospect I believe that it would've been better to hold of on this change for version 2.0, and somewhat regret making it. On the other hand, I also believe there is a difference between your public API, and your stdout log-messages - similar to the difference of git's plumbing and porcelain. I concede that this probably was not a great move on our part though.

But in node.js it's easy enough to avoid any of them using native methods.

Mocha is also supposed to run in the browser, including IE8. That's the whole point of these sims. ;-)

[boneskull]

@rlidwka As @jbnicolai wrote, SemVer does not mention "program output", which is either one of the problems or benefits of SemVer, depending on how you look at it.

If we assume the output is part of the API: the "output" of Mocha is whether or not the test passed or failed; not how it is displayed. Should changing the display of any given console reporter--say, changing some colors--be reserved for major releases?

If we assume the output is not part of the API: the API did not change.

Either way, it wasn't a breaking change, but certainly one that should have been considered more carefully.

@rlidwka Why would you prefer underscore over LoDash?

[rlidwka]

If we assume the output is not part of the API: the API did not change.

Output of command-line app is not part of the API. But command-line arguments are:

-R, --reporter

The --reporter option allows you to specify the reporter that will be used, defaulting to "dot". This flag may also be used to utilize third-party reporters. For example if you npm install mocha-lcov-reporter you may then do --reporter mocha-lcov-reporter.

And changing the default values of command-line arguments is a breaking change. By the way, docs are still saying that default value is "dot".

I used to think that breaking change is a change that forces people who depend on the package to make changes in their programs in the update. Thus, if people are adding -R dot after update, it is one.

No, that's okay I suppose. By the versioning system I'm using, it's qualifies as "minor" as well, because it forces change on the small amount of users. But strictly speaking it's not semver.

I'd even say that the only library I know that follows semver strictly is "request", and all others break it occasionally. My point is: don't judge underscore on this. They do break stuff in minors, but at least they do it openly, so people have some time to adjust.

Why would you prefer underscore over LoDash?

It's way over-engineered to be used in node.js. But I forgot you're targeting browsers as well, maybe it's worth it there, I don't know.

[boneskull]

@rlidwka I'm not going to rebut this; do not feel like arguing about it further.

In what way is LoDash over-engineered, and how is that inappropriate for a NodeJS context?

[boneskull]

(I think we could get a pretty small, compatible build using the custom build tool. Of course, this is your test environment, so I'm not sure we need to lose sleep over bytes.)

[rlidwka]

In what way is LoDash over-engineered, and how is that inappropriate for a NodeJS context?

If you depend on it using npm, it's one megabyte of disk space on all your users' hard drive:

$ npm list
├── lodash@2.4.1
└── underscore@1.7.0

$ du -sh *
976K    lodash
76K     underscore

After seeing 20-line jshttp modules, that thing just looks weird.

And in node.js it's just easier to use native functions. They don't mess up stack traces, they are faster in most of the cases, etc. Recently I was rewriting hike-js trying to optimize for performance, and lodash was the first thing to get rid of. Only function that's hard to replace was sortBy. Was thinking about using underscore just for that one, but in that particular case zero dependencies was preferable.

So basically, I'm saying the same thing as you, it "seems like a rather large dependency" for what it does.

[jbnicolai]

@rlidwka: Thanks for your input, I think we are basically all in agreement that it's not something we need right now. I definitely agree on your points regarding SemVer, and will probably be more careful in the future.

And changing the default values of command-line arguments is a breaking change. By the way, docs are still saying that default value is "dot".

Thanks, will fix. See #1351

@boneskull: do you feel there's still anything to be discussed regarding utils.js? I think @travisjeffery is right that we should probably pick it up when we're already changing it for some other reason, and not change it for the sake of change (although a cleanup is always nice).

[boneskull]

@jbnicolai The whole build process is kind of hinky and part of this has to do with utils.js.

It seems to me that:

1. We don't need a lot of CLI-only stuff in the browser build
2. Compilation itself is a clever albeit egregious hack
3. We should distill the "core" into code that runs in both the browser and the server. Anything else should be decoupled.
4. A sane way to build would probably be using Browserify, but jsfuse is worth a look.
5. The code coverage strategy breaks Browserify, and should probably be replaced with blanket or something, as Jade has done.
6. If we're going to do the above, we might as well remove our own polyfills/shims and get a custom LoDash build going.

So, yes, there's more to discuss. 😄

[boneskull]

If @travisjeffery and @jbnicolai like the idea of building with Browserify, as @jonathanong attempted in #1260, I'd like to pursue it.

[jonathanong]

@boneskull feel free to work off my PR. i think the main issue is that it tries to include ALL the reporters - we should break them out first so we don't try to bundle them in the browser. the alternative is some crazy package.browser config which i'm not really down with

[dougwilson]

i just wanted to chime in here that it would be nice if we could at least keep node.js 0.6 or 0.8 compatibility; the escape-string-regexp specifies that it requires node.js 0.10, which seem weird given what it's contents are.

[boneskull]

@jbnicolai If escape-string-regexp really needs 0.10 or greater, we're gonna have to sit on that until 2.0

[jbnicolai]

Fixed in c869c79 of escape-string-regexp and fe68acb of mocha.

[dougwilson]

Cool 👍 mocha currently works on 0.4. I use it on 0.6 and was wondering what version is going to drop 0.6? Will it just be the next major, or earlier (just asking to know what version range I should depend on).

[boneskull]

I'd like to get some metrics on NodeJS version usage so we can make an educated decision on when to drop old versions

[boneskull]

well, this is cleaned up enough now.