Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($compile): Allow ES6 classes as controllers with bindToController: true #13540

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

Modify $injector.invoke so ES6 classes would be invoked using new

Closes: #13510

@lgalfaso lgalfaso force-pushed the issue-13510 branch 2 times, most recently from 7562677 to 6dd8f17 Compare December 14, 2015 22:56
…er: true`

Modify `$injector.invoke` so ES6 classes would be invoked using `new`

Closes: angular#13510
Closes: angular#13540
@lgalfaso
Copy link
Contributor Author

Do not know how to fix this flake in the tests

IE 9.0.0 (Windows 7) filters date should ignore falsy inputs FAILED
    Error: [$injector:modulerr] Failed to instantiate module ng due to:
    Out of memory

},
controller: eval(
"class Foo {" +
" constructor($scope) {}" +
Copy link
Member

Choose a reason for hiding this comment

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

Is $scope needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, will remove it

@gkalpak
Copy link
Member

gkalpak commented Dec 15, 2015

LGTM

Something for a follow-up PR:
Maybe it's a good idea to avoid intializing the bindings prior to instantiating a class-based controller, since we know they won't be used (i.e. the return value will be different than self).

@lgalfaso
Copy link
Contributor Author

@petebacondarwin do you think this should go into 1.5 ?

@bisubus
Copy link

bisubus commented Jan 3, 2016

@lgalfaso There's a significant problem with this approach, I've covered it in this comment and fought it in that extension (you may go by the implementation and see how clumsy it became with multiple checks and flags).

The source of the problem is that this

function isClass(func) {
  return typeof func === 'function'
    && /^class\s/.test(Function.prototype.toString.call(func));
}

will work for native classes and will fail for transpiled ones (with Babel).

@lgalfaso lgalfaso closed this in b0248b7 Jan 3, 2016
@lgalfaso
Copy link
Contributor Author

lgalfaso commented Jan 3, 2016

I am landing this as this PR already has enough to handle all current browsers that natively support class.

@lgalfaso
Copy link
Contributor Author

lgalfaso commented Jan 3, 2016

@bisubus I am not sure how this affects Babel at all. The current mechanism that forces a this into the constructor function is indistinguishable for Babel as there is no support for new.target. Would it be possible for you to create a plunker that uses the latest snapshot and shows this issue?

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jan 4, 2016
…er: true`

Modify `$injector.invoke` so ES6 classes would be invoked using `new`

Closes: angular#13510
Closes: angular#13540 (reverted from commit b0248b7)
@petebacondarwin
Copy link
Contributor

This change caused IE9 to run out of memory.

@petebacondarwin petebacondarwin added this to the 1.5.0-rc.1 milestone Jan 4, 2016
lgalfaso added a commit to lgalfaso/angular.js that referenced this pull request Jan 5, 2016
…er: true`

Modify `$injector.invoke` so ES6 classes would be invoked using `new`

Closes: angular#13510
Closes: angular#13540
@lgalfaso lgalfaso closed this in 8955cfb Jan 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants