Feature/es6 class constructor support

[OJezu]

Adds support for constructing es6 class instances. Fixes #181

Tests do not pass at this moment to highlight the non-backwards compatible change in automatic constructor detection. In order to accurately detect es6 classes I added check whether functions have a "constructor" property. As I have no better idea how to detect those classes, it's either this or explicit isConstructor will be needed in all components loading es6 classes.

When a decision is made how to approach this, I will adjust tests to conform to it.

[OJezu]

Tests on older versions of node are failing in some unexpected ways, I will have to debug it further.

[briancavalier]

Wow, thanks for digging into this @OJezu. I'll try to take a look tonight.

[OJezu]

The tests on older versions of node were failing because of my adjustments to isConstructor function:

function isConstructor(func) {
  var is = false, p;

   ***is = is || !!func.constructor;***

  if(!is) {
    (...)

I removed that check, although now I'm unable to detect es6 classes automatically.

[OJezu]

I've added detection of class functions back, this time using func.toString() - according to spec it has to work.

[OJezu]

I don't want to modify the prototype from here, so I set the constructor property on object itself.

[briancavalier]

Yep, looks right. Nice find. Any idea of the performance characteristics of this? I figure it's probably fine, but just want to make sure we're not introducing a big perf hit by toStringing every created function.

[briancavalier]

This looks good @OJezu. Really awesome that you created separate tests for es6, too. Just one question about performance. Is there anything else you feel needs to be done before we merge?

[OJezu]

I did not test the code in any browsers, it would be good if someone would check if their web app is not broken by this in major ones. Performance-wise I don't see potential slowdowns, maybe except toString on big functions.

Dnia 12 kwietnia 2016 13:28:55 CEST, Brian Cavalier [email protected] napisał(a):

This looks good @OJezu. Really awesome that you created separate tests
for es6, too. Just one question about performance. Is there anything
else you feel needs to be done before we merge?

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#182 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[OJezu]

Oh, and toString is used only in modern, es6 capable browsers. Hopefully, that means optimised browsers.

Dnia 12 kwietnia 2016 13:42:02 CEST, Krzysztof Chrapka [email protected] napisał(a):

I did not test the code in any browsers, it would be good if someone
would check if their web app is not broken by this in major ones.
Performance-wise I don't see potential slowdowns, maybe except toString
on big functions.

Dnia 12 kwietnia 2016 13:28:55 CEST, Brian Cavalier
[email protected] napisał(a):

This looks good @OJezu. Really awesome that you created separate
tests
for es6, too. Just one question about performance. Is there anything
else you feel needs to be done before we merge?

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#182 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[briancavalier]

it would be good if someone would check if their web app is not broken by this in major ones

Yeah, IE might be a concern. I'm not too worried about FF, Chrome, and Safari. I don't have direct access to IE, but we could potentially setup a simple test case, and use saucelabs or some other service to smoke test it. What do you think?

toString is used only in modern, es6 capable browsers.

Nice.

[OJezu]

I have IE in an virtual machine, but I don't have an app to test it.

Dnia 12 kwietnia 2016 13:54:39 CEST, Brian Cavalier [email protected] napisał(a):

it would be good if someone would check if their web app is not
broken by this in major ones

Yeah, IE might be a concern. I'm not too worried about FF, Chrome, and
Safari. I don't have direct access to IE, but we could potentially
setup a simple test case, and use saucelabs or some other service to
smoke test it. What do you think?

toString is used only in modern, es6 capable browsers.

Nice.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#182 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[briancavalier]

Could you write a test similar to the other browser tests, e.g. this one and use that to verify in your VM?

[OJezu]

I'm under a pile of work, I should be able to do the testing at the beginning of the next week, or over the weekend.

[briancavalier]

Now worries, @OJezu, I know the feeling!

[OJezu]

I'm unable to run tests in browsers. Wire seems to depend on some old version of doh for this, which is not included in npm package. If I install doh from bower, I have to update dojo too, and then everything fails on no define function defined.

[OJezu]

I'm trying to run the tests on master, to be clear:

with bundled version of dojo:

with new version of dojo:

[OJezu]

I've made buster node tests runnable in browser. I had to disable some of them in browser, because gent does not work with AMD. I have yet to change way in which it is decided that es6-tests are to be loaded to browsers, and fix a bug in IE. I don't even know if it was my code which introduced it, because there is no way to test previous version of code.

[OJezu]

@briancavalier sorry for 20 day delay, but I found reason why this test was not passing in IE (turns out Function.name was non-standard 'till ES2015!), and now everything is passing in Chromium 49, FF 38 (Iceweasel) and IE 11. Let me know if you are ok with test modifications. If not, please advise what to do to make old tests runnable.