Skip to content

Commit

Permalink
According to the spec, "undefined" as the mapFn arg should not throw.
Browse files Browse the repository at this point in the history
  • Loading branch information
ljharb committed Sep 7, 2014
1 parent 4218d77 commit 3ac0659
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 2 deletions.
2 changes: 1 addition & 1 deletion es6-shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@
var mapFn = arguments.length > 1 ? arguments[1] : undefined;

var list = ES.ToObject(iterable, 'bad iterable');
if (arguments.length > 1 && !ES.IsCallable(mapFn)) {
if (mapFn !== undefined && !ES.IsCallable(mapFn)) {
throw new TypeError('Array.from: when provided, the second argument must be a function');
}

Expand Down
5 changes: 4 additions & 1 deletion test/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,11 @@ var runArrayTests = function() {
});
});

it('does not throw when provided an undefined second arg', function() {
expect(Array.from([], undefined)).to.eql([]);
});

it('throws when provided a nonfunction second arg', function() {
expect(function () { Array.from([], undefined); }).to.throw(TypeError);
expect(function () { Array.from([], null); }).to.throw(TypeError);
expect(function () { Array.from([], false); }).to.throw(TypeError);
expect(function () { Array.from([], true); }).to.throw(TypeError);
Expand Down

4 comments on commit 3ac0659

@cscott
Copy link
Collaborator

@cscott cscott commented on 3ac0659 Sep 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I think they changed the spec here. Could you double check the line before it: var mapFn = arguments.length > 1 ? arguments[1] : undefined; -- I think they changed that to ignore arguments.length and just look at the value as well.

@ljharb
Copy link
Collaborator Author

@ljharb ljharb commented on 3ac0659 Sep 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good call. unfortunately checking arguments indexes that are larger than the length severely deoptimizes the JIT in v8, so it will incur a performance hit when it's not supplied. :-/

@ljharb
Copy link
Collaborator Author

@ljharb ljharb commented on 3ac0659 Sep 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually - to my reading, the difference is that if it's supplied as literal undefined, that should be treated the same as if it wasn't supplied - whereas for thisArg, if it's supplied as undefined, that's different than if it's not supplied.

So, I think the var mapFn = line is correct as-is.

@cscott
Copy link
Collaborator

@cscott cscott commented on 3ac0659 Sep 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the arguments check should be harmless in terms of semantics, and it prevents deoptimization that's all good.

Please sign in to comment.