Migrate to ES2015 #2948

devoto13 · 2018-03-05T01:49:40Z

So this is another take on migrating code base to ES2015 classes, const/let and arrow functions. With Node 4 reaching end-of-life in 2 months developers soon can enjoy full support of ES2015 😄

Using mentioned syntaxes should give cleaner, more structured and therefore more maintainable code.

I originally tried to base my work on #2904 from @EzraBrooks, but rebasing on master turned out to be very nasty, so I gave up and started over.

The goal of this PR is exclusively migration to ES2015 and NOT change of a DI mechanism. I monkey-patched di module to support ES classes for now. Everything should work exactly the same as it worked for functions. The DI migration can be done later (if needed).

I'm looking for confirmation that this approach is fine by maintainers and than I will continue to refactor more classes.

johnjbarton · 2018-03-06T17:20:06Z

lib/browser.js

+  }
+
+  bindSocketEvents (socket) {
+    // TODO: check which of these events are actually emitted by socket


I think this change, while probably a good direction, should be separate so we don't get sources of errors mixed up.

Methods in class declarations are not enumerable properties, so bindAll does not work over ES2015 classes... We'll either have to change bindAll implementation somehow or do this one. I think being explicit here is better (it took me some time to figure out why it was failing). I can extract this into separate commit explaining the change if you prefer it?

Yes, thanks, I do think making this change separately and before the classes would be good strategy.

Extracted this change into a separate commit: a51505a.

Thanks! I think for a change of this magnitude and one that (necessarily) differs in style we should have other folks like @dignifiedquire weigh in. The 'browser' changes look good to me.

I am all for the explicit version 👍

johnjbarton · 2018-03-06T17:31:53Z

lib/helper.js

@@ -166,3 +166,64 @@ exports.defer = function () {
    promise: promise
  }
 }
+
+/**
+ * Patches di module to support ES2015 classes.


While I think it is cool that you worked this out, I am not keen on this strategy and this implementation. The karma-special "DI" system makes karma difficult to work with until one spends time decoding what it does. To me, this DI patching approach makes the problem worse: now we have another layer of confusion hiding the first confusion.

The migration to ES classes will make the karma code easier to read for most developers. Can we find a way to achieve that without the DI patching?

The karma DI works through instances exported and matched with some funky $inject coding. The conversion to classes changes source which is fundamentally singleton into class structure, which then requires object creation to fit back into the DI system. Rather than hacking the DI system to create instances from exported classes, consider leaving the DI and module interfaces unchanged in this first pass. Create instances in the new ES modules, export them just as karma does now. That gives us ES class support without obscuring the obscure. It also avoids bundling changes that will make bug tracking very difficult. Then we can tackle the DI issue separately.

Well, after reading the source code of di module I find it very straightforward and simple to understand DI implementation 😄 And the fact that it worked all this time without breaking only confirms the fact. Not sure why users are having difficulties working with it, maybe a bit of documentation can completely resolve the problem.

I doubt there is a much better/more type safe dependency injection mechanism possible using plain JS at the moment.

Can we find a way to achieve that without the DI patching?

I think I can convert all instantiate() calls and type providers to use invoke() and factory methods instead and it will work without a DI patch. I'll check it later today.

Something like here:

- capturedBrowsers: ['type', BrowserCollection], + capturedBrowsers: ['factory', function (emitter) { return new BrowserCollection(emitter) }],

Documentation would be awesome.

I agree that the DI system has proven useful. Nevertheless it is unusual and unique, requiring users to read the code for a probably unrelated subsystem to make progress on changes. For your change however, the issue is only to not make things more complex if we can achieve your goal within the current system.

I've removed a di patch and replaced with factory functions.

johnjbarton · 2018-03-08T17:18:32Z

The changes look good to me. Looks like the last one tripped some issue in the continuous integration tests.

devoto13 · 2018-03-08T21:22:42Z

@johnjbarton the issue is in all commits actually. Tests fail in Node 4 only. As I pointed in the original post:

With Node 4 reaching end-of-life in 2 months developers soon can enjoy full support of ES2015 😄

So I was hoping to get this in together with dropping Node 4 support. I can probably try to support Node 4 as well, but not sure if it worths the effort given that it is reaching end-of-life on 30th of April and has quite poor ES2015 support.

dignifiedquire · 2018-03-08T22:34:30Z

I don't think we should drop Node 4 support that soon, so I would like for now to make sure it works on Node 4.

dignifiedquire · 2018-03-08T22:36:53Z

@devoto13 also if you could simply add strict mode to all files we should be good even on Node 4 (strict mode is preferable anyway)

devoto13 · 2018-03-12T08:44:22Z

@dignifiedquire rest/spread is not supported in the Node 4 either... And it is very convenient alternative to using arguments and fn.apply(). But ok, I'll make sure it works on Node 4 as well.

devoto13 · 2018-03-14T22:00:59Z

I've updated to support Node 4. One test is failing in Firefox 59 not because of my changes. There is same error in master branch. Any ideas how to fix it?

devoto13 · 2018-03-14T23:23:02Z

Turns out the not-standard version property was removed in Firefox 59. From MDN:

Note: in Firefox you could specify the version of JavaScript contained in a <script> element by including a non-standard version parameter inside the type attribute — for example type="application/javascript;version=1.8". This has been removed in Firefox 59 (see bug 1428745).

I've added quick fix to ignore it in Firefox 59+ and deprecation warning for the jsVersion property. Do you think it is the right way to handle it? If so I will submit it in a separate PR.

johnjbarton · 2018-03-14T23:51:04Z

fix to ignore it in Firefox 59+ and deprecation warning for the jsVersion property.

Yes, in a separate PR.

dignifiedquire

LGTM thanks for doing this, once the firefox issue is fixed on master, this is rebased ontop and CI is green I'll merge it

This makes code easier to understand, because all listeners are added explicitly. Now it is clear what methods are called in response to which events and what parameters are passed to them. Also it is a preparation for migration to ES2015 classes, where original approach won't work, because methods in ES2015 classes are added as not enumerable properties, so it is harder to loop through them.

devoto13 force-pushed the es2015 branch from 314ea55 to 07df095 Compare March 6, 2018 00:25

johnjbarton reviewed Mar 6, 2018

View reviewed changes

devoto13 force-pushed the es2015 branch from 07df095 to a89339e Compare March 7, 2018 00:33

dignifiedquire added the needs: feedback-from-author label Mar 8, 2018

devoto13 force-pushed the es2015 branch from a89339e to 7e21a43 Compare March 14, 2018 20:38

dignifiedquire approved these changes Mar 15, 2018

View reviewed changes

devoto13 added 9 commits March 15, 2018 12:22

refactor: migrate Browser class to ES2015

bb012e2

refactor: migrate BrowserCollection class to ES2015

5809653

refactor: migrate BrowserResult class to ES2015

33b1078

refactor: migrate cli to ES2015

aeb4541

refactor: migrate complection to ES2015

a1f4716

refactor: migrate config to ES2015

75ec567

refactor: migrate misc files to ES2015

9b472c4

refactor: migrate EmitterWrapped to ES2015

0e6d46c

devoto13 force-pushed the es2015 branch from 526685a to 0e6d46c Compare March 15, 2018 11:27

dignifiedquire merged commit 2270abb into karma-runner:master Mar 15, 2018

EzraBrooks mentioned this pull request May 24, 2018

refactor: transition from pseudoclasses to ES6 classes #2904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to ES2015 #2948

Migrate to ES2015 #2948

devoto13 commented Mar 5, 2018

johnjbarton Mar 6, 2018

devoto13 Mar 6, 2018 •

edited

johnjbarton Mar 6, 2018

devoto13 Mar 7, 2018

johnjbarton Mar 7, 2018

dignifiedquire Mar 8, 2018

johnjbarton Mar 6, 2018

devoto13 Mar 6, 2018 •

edited

johnjbarton Mar 6, 2018

devoto13 Mar 7, 2018

johnjbarton commented Mar 8, 2018

devoto13 commented Mar 8, 2018

dignifiedquire commented Mar 8, 2018

dignifiedquire commented Mar 8, 2018

devoto13 commented Mar 12, 2018 •

edited

devoto13 commented Mar 14, 2018

devoto13 commented Mar 14, 2018

johnjbarton commented Mar 14, 2018

dignifiedquire left a comment

Migrate to ES2015 #2948

Migrate to ES2015 #2948

Conversation

devoto13 commented Mar 5, 2018

Choose a reason for hiding this comment

devoto13 Mar 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoto13 Mar 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjbarton commented Mar 8, 2018

devoto13 commented Mar 8, 2018

dignifiedquire commented Mar 8, 2018

dignifiedquire commented Mar 8, 2018

devoto13 commented Mar 12, 2018 • edited

devoto13 commented Mar 14, 2018

devoto13 commented Mar 14, 2018

johnjbarton commented Mar 14, 2018

dignifiedquire left a comment

Choose a reason for hiding this comment

devoto13 Mar 6, 2018 •

edited

devoto13 Mar 6, 2018 •

edited

devoto13 commented Mar 12, 2018 •

edited