Refactoring: use ES5.1 features #1271

mroderick · 2017-02-19T22:49:03Z

Purpose (TL;DR) - mandatory

Refactor code to use ES5.1 features and avoid many for and for..in loops

How to verify - mandatory

Check out this branch (see github instructions below)
npm install
npm test

Note

This PR is not ready to be merged yet, I haven't removed all the loops yet.

I would like to group the changes better into fewer commits (related changes together), but my brain is too tired to finish it tonight.

lucasfcosta · 2017-02-20T00:06:38Z

lib/sinon/match.js

@@ -109,7 +107,7 @@ var TYPE_MAP = {
 };

 function match(expectation, message) {
-    var m = create(matcher);
+    var m = Object.create(matcher);


The whole code looks much better and less error prone by using a native function instead of reimplementing it.
Well done!

lucasfcosta · 2017-02-20T00:09:13Z

lib/sinon/mock-expectation.js

-        for (var i = 0, l = this.expectedArguments.length; i < l; i += 1) {
-
-            if (!verifyMatcher(this.expectedArguments[i], args[i])) {
+        expectedArguments.forEach(function (expectedArgument, i) {


This looks a lot cleaner too. 👍

lucasfcosta · 2017-02-20T00:20:25Z

Awesome work!
The code looks a lot more clear, especially when you substituted the old implementations for the native ones.
Its impressive to see how cleaner it gets when we start using functions instead of regular control structures. It gets even more interesting when you compare it to languages like Go.

Anyway, philosophical reflections aside, this looks awesome.

fatso83

Everything looks good, just wondering about the every bit.

fatso83 · 2017-02-20T08:17:32Z

lib/sinon/match.js

@@ -8,7 +8,6 @@
 */
 "use strict";

-var create = require("./util/core/create");
 var deepEqual = require("./util/core/deep-equal").use(match); // eslint-disable-line no-use-before-define
 var every = require("./util/core/every");


Why can't we remove every use of every as well in favor of Array#every? Are they not doing the same thing?

I am working on it :)

It'll require a bit of refactoring of the code, as the every function is designed to also work with Object, because someone decided that they are "iterables"....

Yeah, I remember we came to this conclusion when we talked about Array-like objects and Maps and Sets.

In this comment I talk about it. Actually this method is really useful when it comes to iterating through Map and Set objects in order to be able to check all of their elements for our related matchers. It is not a simple re-implementation of the original every function, it is an slightly modified version to work with these kinds of objects.

Instead use Object.keys defined in ES5.1 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys

Object.create is defined in ES5.1 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

ES5.1 defines Array.prototype.filter

mantoni · 2017-02-20T14:04:52Z

lib/sinon/behavior.js

@@ -377,11 +377,11 @@ function createAsyncVersion(syncFnName) {
 }

 // create asynchronous versions of callsArg* and yields* methods
-for (var method in proto) {
+Object.keys(proto).forEach(function (method) {
    // need to avoid creating anotherasync versions of the newly added async methods
    if (proto.hasOwnProperty(method) && method.match(/^(callsArg|yields)/) && !method.match(/Async/)) {


You can remove the hasOwnProperty check here.

I've pushed a review commit that removes it. Once we're happy with the PR, I'll rebase and squash this commit

mantoni · 2017-02-20T14:06:40Z

lib/sinon/call.js

-        var l = arguments.length;
-        if (l > this.args.length) {
+        var self = this;
+        var calledWithArgs = Array.prototype.slice.call(arguments);


There is a slice variable at the top of this file because it's used so frequently. Do you prefer using the lengthy version to be more explicit?

I've pushed a commit that uses the slice convenience variable

ES5.1 defines Array.prototype.reduce https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce

ES5.1 defines Array.prototype.every https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every

ES5.1 defines Array.prototype.map https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map

mroderick · 2017-02-24T10:58:07Z

I have rebased and squashed the changes from code review into the relevant commits so the history reads better. I think this is good to merge.

I would like to get this merged before the weekend, so we will have a cleaner codebase to work with.

coveralls · 2017-02-24T10:58:31Z

Coverage decreased (-0.009%) to 94.856% when pulling 44831d4 on mroderick:use-es5-features into afdf425 on sinonjs:master.

mantoni

LGTM 👍

mroderick · 2017-02-24T11:14:10Z

Thanks

erikpukinskis · 2017-03-15T22:51:50Z

Thank you everyone for your hard work on Sinon these years! It's been great for me, but now that you are leaving Node for IO.js, I must part ways! You were one of the last few modules to keep supporting Node, which I appreciate greatly.

fatso83 · 2017-03-15T23:13:38Z

@erikpukinskis what are you referring to? Doesn't make much sense to me. All code in Sinon should run on pretty much any JS engine for the last 7 years as we only rely on ES5.1 constructs. And IO.js was merged into Node and no longer exists so...

erikpukinskis · 2017-03-17T20:08:34Z

Hm. I think I got confused because sinon-chai hasn't been ported to sinon-2.0 yet, so I was getting npm errors, and I'm used to being dropped for runtime reasons! Looks like I can live a little longer on my old systems!

I am still on old pre-IO.js versions of Node. I don't support the direction the ES6 community is going. I think it's important to maintain support for the old JavaScript runtimes. I appreciate your efforts at backwards compatibility.

lucasfcosta reviewed Feb 20, 2017

View reviewed changes

fatso83 reviewed Feb 20, 2017

View reviewed changes

mroderick added 11 commits February 20, 2017 09:31

Remove commented out code

ceddcfd

Remove IE6 specific code

e135bfd

Remove object-keys.js

4449426

Instead use Object.keys defined in ES5.1 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys

Use Object.create instead of lib/sinon/util/core/create.js

221ddda

Object.create is defined in ES5.1 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

Simplify restoreContext by using a variable

3057b73

Remove silly compact function

a93e449

Reduce indentation by returning early

c6a433b

Reduce indentation by returning early

609a8e4

Create local variable to reduce complexity

16fae3b

Replace for loop with Array.prototype.filter

7befdd0

ES5.1 defines Array.prototype.filter

Replace for loop with Array.prototype.every

8a92353

mroderick force-pushed the use-es5-features branch from 931e667 to 4b4d0c0 Compare February 20, 2017 08:47

mantoni reviewed Feb 20, 2017

View reviewed changes

mroderick added 11 commits February 24, 2017 11:53

Replace for loop with Array.prototype.forEach

1e4e13b

Replace for loop with Array.prototype.indexOf

92089a9

Replace for loop with Array.prototype.map

0094a8f

Replace for loop with Array.prototype.reduce

2a6dae5

ES5.1 defines Array.prototype.reduce https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce

Replace for..in with Array.prototype.every

7d854f1

ES5.1 defines Array.prototype.every https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every

Replace for..in with Array.prototype.filter

ee6d008

Replace for..in loop with Array.prototype.forEach

671461b

Replace for..in with Array.prototype.map

ba9e411

ES5.1 defines Array.prototype.map https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map

Replace for..in with Array.prototype.reduce

d44337a

Replace homegrown some with Array.prototype.some

8b716bd

Replace homegrown each with Array.prototype.forEach

44831d4

mroderick force-pushed the use-es5-features branch from 4b38568 to 44831d4 Compare February 24, 2017 10:56

mantoni approved these changes Feb 24, 2017

View reviewed changes

mroderick merged commit 6b13ed7 into sinonjs:master Feb 24, 2017

mroderick deleted the use-es5-features branch February 24, 2017 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring: use ES5.1 features #1271

Refactoring: use ES5.1 features #1271

mroderick commented Feb 19, 2017

lucasfcosta Feb 20, 2017

lucasfcosta Feb 20, 2017

lucasfcosta commented Feb 20, 2017

fatso83 left a comment

fatso83 Feb 20, 2017

mroderick Feb 20, 2017

mroderick Feb 20, 2017

lucasfcosta Feb 21, 2017 •

edited

Loading

mantoni Feb 20, 2017

mroderick Feb 20, 2017

mantoni Feb 20, 2017

mroderick Feb 20, 2017

mroderick commented Feb 24, 2017

coveralls commented Feb 24, 2017 •

edited

Loading

mantoni left a comment

mroderick commented Feb 24, 2017

erikpukinskis commented Mar 15, 2017 •

edited

Loading

fatso83 commented Mar 15, 2017

erikpukinskis commented Mar 17, 2017 •

edited

Loading

Refactoring: use ES5.1 features #1271

Refactoring: use ES5.1 features #1271

Conversation

mroderick commented Feb 19, 2017

Purpose (TL;DR) - mandatory

How to verify - mandatory

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Feb 20, 2017

fatso83 left a 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

lucasfcosta Feb 21, 2017 • edited Loading

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

mroderick commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

mantoni left a comment

Choose a reason for hiding this comment

mroderick commented Feb 24, 2017

erikpukinskis commented Mar 15, 2017 • edited Loading

fatso83 commented Mar 15, 2017

erikpukinskis commented Mar 17, 2017 • edited Loading

lucasfcosta Feb 21, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

erikpukinskis commented Mar 15, 2017 •

edited

Loading

erikpukinskis commented Mar 17, 2017 •

edited

Loading