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

ES6 Refactor #444

Closed
tejasmanohar opened this issue Sep 27, 2015 · 14 comments
Closed

ES6 Refactor #444

tejasmanohar opened this issue Sep 27, 2015 · 14 comments
Labels

Comments

@tejasmanohar
Copy link

@tejasmanohar tejasmanohar commented Sep 27, 2015

Would Webtorrent consider an ES6 refactor if it was sent as a PR? Maybe not all of it at once but part-by-part.

Would probably use Babel for transpiling but could use shims instead.

@feross feross added the question label Sep 27, 2015
@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Sep 27, 2015

Thanks for your enthusiasm to contribute to WebTorrent!

Sorry, but I don't think that it's worth adding a compilation step and using features that don't work in most node.js environments for a bit of synatic sugar. Maybe in a year or two when ES6 is available to the majority of node.js users (i.e. they are running iojs or node v4). Right now, I believe 0.10 is still the most widely used version.

If you're looking for a good first issue to work on, take a look at the "good first contribution" and "help wanted" tags. Thanks again! 👍

@feross feross closed this Sep 27, 2015
@wtgtybhertgeghgtwtg

This comment has been minimized.

Copy link

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Oct 14, 2016

Well, it's been a year and webtorrent is strictly for node v4 and up.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

@DiegoRBaquero DiegoRBaquero commented Oct 14, 2016

Sept 28, 2016:

@deckar01

This comment has been minimized.

Copy link

@deckar01 deckar01 commented Nov 1, 2016

I don't think that it's worth ... using features that don't work in most node.js environments ...

Maybe in a year or two when ES6 is available to the majority of node.js users (i.e. they are running iojs or node v4).

@feross It looks like WebTorrent has required Node 4+ since August. c567e67

I don't think that it's worth adding a compilation step ...

All the browsers that WebTorrent supports also seem to have mature ES6 support, so no compiling would be necessary.

I don't think that it's worth ... using ... for a bit of synatic sugar.

The arrow functions and classes alone improve the maintainability and readability of a code base enough for me to justify using ES6 features.

Another advantage would be the advanced tree shaking that is possible with ES6 style import statements. Since WebTorrent is a rather large code base, it would be really nice to let rollup walk the AST and remove the parts of WebTorrent I am not using.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

@DiegoRBaquero DiegoRBaquero commented Nov 1, 2016

October 31st, 2016:

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jan 21, 2017

We switched to requiring Node v4 because the version of npm in older versions of node couldn't install git dependencies.

ES6 is cool, but it's not worth completely breaking the demos on https://webtorrent.io for IE11 and the the previous version of Safari (9) just for a little syntactic sugar. There are also lots of users depending on the fallback to web seeds in browsers without WebRTC support.

If we gain something from the switch, like being able to use a dependency we really want/need, then I'm okay with ES6 in this codebase.

The tree shaking argument is interesting, but I'm skeptical that webtorrent's size can be reduced much. I've combed through the codebase meticulously to minimize file size. As it stands, we have an entire torrent client in 77kb, which is really great as far as I'm concerned 👍

import would also require us to use babel for node/browser support and that's too much complexity to layer on right now.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jan 21, 2017

That said, if you want to contribute to WebTorrent development and insist on not sullying your fingers with any ES5 keystrokes, you can check out webtorrent-desktop, a pure ES6+ codebase.

@deckar01

This comment has been minimized.

Copy link

@deckar01 deckar01 commented Jan 31, 2017

@feross I whipped up a proof of concept and pushed it to deckar01/webtorrent/444-es6. I don't expect you to do anything with it, but I may merge in upstream changes occasionally.

Here is a summary of how I found the new ES6 syntax to be useful:

  • Replacing the inherits module with native class inheritance
  • Replacing self = this statements with arrow functions
  • Replacing Class.prototype.method = function boilerplate with nested class methods
  • Replacing complex string concatenation with template literals
  • Replacing var hoisting with block level let declarations

There were some other refactoring opportunities I found that did not rely on ES6 features. There are some excessively nested closures that are much easier to understand once they are decoupled into class methods.

I got the tests passing on Firefox and Chrome, but I couldn't get sauce to start IE. I found a thread where you had a similar issue in the past. Any help would be appreciated.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

@DiegoRBaquero DiegoRBaquero commented Jan 31, 2017

Jan 5, 2017:

I don't want to use Babel, requiring NodeJS 6+ is a bit too much right now. NodeJS 4 lacks many ES6 features.

@wtgtybhertgeghgtwtg

This comment has been minimized.

Copy link

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Jan 31, 2017

node@4.0.0 has class, let, const, and arrow functions. Those are some of the bigger ones.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

@DiegoRBaquero DiegoRBaquero commented Feb 1, 2017

Jan 31, 2017:

Does anyone has stats on market share of ES6 compatible browsers?

@wtgtybhertgeghgtwtg

This comment has been minimized.

Copy link

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Feb 1, 2017

I'm not sure if market share is the right question. webtorrent can't be used if the browser doesn't support WebRTC, right? So it already doesn't work for IE and Edge.

@lock

This comment has been minimized.

Copy link

@lock lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jul 27, 2018

Continued here: #1443

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.