Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/es6 class constructor support #182

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OJezu
Copy link
Contributor

@OJezu OJezu commented Apr 11, 2016

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 OJezu force-pushed the feature/es6-class-constructor-support branch from 47f50d0 to 81e44b8 Compare April 11, 2016 13:52
@OJezu
Copy link
Contributor Author

OJezu commented Apr 11, 2016

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

@briancavalier
Copy link
Member

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

@OJezu OJezu force-pushed the feature/es6-class-constructor-support branch from 81e44b8 to b365c09 Compare April 11, 2016 15:30
@OJezu
Copy link
Contributor Author

OJezu commented Apr 11, 2016

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 OJezu force-pushed the feature/es6-class-constructor-support branch 3 times, most recently from 4d1d6a9 to 2eaa2b7 Compare April 12, 2016 08:28
@OJezu
Copy link
Contributor Author

OJezu commented Apr 12, 2016

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


// detect broken old prototypes with missing constructor
if (result.constructor !== func) {
Object.defineProperty(result, 'constructor', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@OJezu OJezu force-pushed the feature/es6-class-constructor-support branch from 2eaa2b7 to 717abda Compare April 12, 2016 09:24

// this has to work, according to spec:
// https://tc39.github.io/ecma262/#sec-function.prototype.tostring
is = is || func.toString().trim().substr(0,5) === 'class';
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

OJezu commented Apr 12, 2016

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
Copy link
Contributor Author

OJezu commented Apr 12, 2016

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
Copy link
Member

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
Copy link
Contributor Author

OJezu commented Apr 12, 2016

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
Copy link
Member

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

@OJezu
Copy link
Contributor Author

OJezu commented Apr 21, 2016

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
Copy link
Member

Now worries, @OJezu, I know the feeling!

@OJezu
Copy link
Contributor Author

OJezu commented Apr 23, 2016

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
Copy link
Contributor Author

OJezu commented Apr 23, 2016

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

with bundled version of dojo:
with bundled version of dojo

with new version of dojo:
with new version of dojo

@OJezu OJezu force-pushed the feature/es6-class-constructor-support branch from 717abda to 1d7945d Compare April 23, 2016 23:10
@OJezu
Copy link
Contributor Author

OJezu commented Apr 23, 2016

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.

screenshot from 2016-04-24 01 13 49

@OJezu OJezu force-pushed the feature/es6-class-constructor-support branch 2 times, most recently from 8497fe8 to 62a7959 Compare April 24, 2016 00:21
@OJezu OJezu force-pushed the feature/es6-class-constructor-support branch from 62a7959 to a51d2a1 Compare May 13, 2016 14:44
@OJezu
Copy link
Contributor Author

OJezu commented May 13, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants