Bound constructor functions can't be used as controllers

[renenielsendk]

var app = angular.module('nojs', []);

app.controller('nojs', [controller.bind(null)]); // not working
app.controller('nojs', [controller]); // working

function controller(){};

[pkozlowski-opensource]

@Renetnielsen what is the version of AngularJS that gives you problems? Did it work with any previous version (as the issue title would suggest)? If so, what was the version where it was working?

But most importantly - what are you trying to achieve with controller.bind(null)? What is your use-case?

[renenielsendk]

Hi @pkozlowski-opensource before i used 1.2.23 angular npm (working).
Now i upgraded to 1.3.9 (not working).
Ive read in the changelog (https://docs.angularjs.org/guide/migration#angular-expression-parsing-parse-interpolate-). Has it anything to do with this?

I was trying to bind default arguments to a function outside scope

module.exports = function( opts ){
    return [
        '$scope', 
        controller.bind(null, opts)
    ]
}

function controller( opts ){}

[pkozlowski-opensource]

So, the change in behaviour comes from perf-related commit: bf6a79c

The bottom line here is that bound constructor functions don't have prototype property and the Object.create function fails on the undefined prototypes (it is quite happy with null, though).

It is kind of corner case but let me play with it for a sec - maybe the previous behaviour can be restored without too much pain.

[caitp]

Bound functions are not constructors, per JavaScript. You cannot call them with new

[pkozlowski-opensource]

@caitp we are currently using Object.create to create instances - but you are right that those are not real constructors.

[pkozlowski-opensource]

Soooo, it seems like a fix is trivial and boils down to adding a check for undefined prototypes every time we are creating a controller instance, ex.: Object.create(controllerPrototype); => Object.create(controllerPrototype || null);

But the question is - do we want to support this? Personally I see no harm in supporting controller instances without prototypes, the fix is trivial and it looks like it worked in the past, so why not?

@caitp @petebacondarwin WDYT? I can put together a quick PR to see impact on the code (should be fairly minimal, at the first look)

[caitp]

I don't think we should support bound functions, they are not constructors

[pkozlowski-opensource]

@caitp one more thing - it seems like you actually can call bound constructor function with new. This works OK:

var Foo = function(name) { 
    this.name = name; 
}

var BoundFoo = Foo.bind(null);

var boundFoo = new BoundFoo('Me');

console.log(f.name); // prints "Me"

MDN is also suggesting that it should be just fine:

A bound function may also be constructed using the new operator: doing so acts as though the target function had instead been constructed. The provided this value is ignored, while prepended arguments are provided to the emulated function.

Unless I'm misunderstanding what you / they are saying...

[pkozlowski-opensource]

@caitp what would be your argument for not supporting them in the light of the above comment?

[caitp]

Pawell, bound functions are per spec not constructors, e.g. IsConstructor will return false, and standard Library functions expecting constructors will throw

[pkozlowski-opensource]

@caitp hmm, OK, this starts to get interesting as one can create new instances... So the definition of the "pure constructor" seems to be a bit blurry...

Once again, I don't need this functionality myself but don't see much harm in supporting it, especially that it used to work before.

Anyway, did the investigation, can send the PR so I will let @petebacondarwin do the final call on this one. Thnx for all your input @caitp !

[caitp]

At least this was true a while ago, let me verify behavior with spider monkey in a bit

[pkozlowski-opensource]

@caitp sure. I've just tested on all the browsers I've got (Chrome, FFox, Safari) and I can create new instances using new from bound constructor functions without problems. This is why I find this interesting...

[caitp]

Array.of and Array.from used to throw for bound functions, but that appears to no longer be the case, and the text of the draft has changed, so I guess we should support this after all.

That seems to be a trivial bug to fix in v8 if you want to have a go at it

[pkozlowski-opensource]

@caitp not sure I got this comment:

That seems to be a trivial bug to fix in v8 if you want to have a go at it

Are you referring to handling both undefined and null as equivalent? If so we would have to "fix" all the browsers as FFox seems to be upset about undefined as well.

Anyway, sent #10790 to work-arround this thing on AngularJS side.

[caitp]

Are you referring to handling both undefined and null as equivalent

no, I'm referring to the various emulations of IsConstructor() doing the wrong things