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

Refactor lib/* in ES6? #2426

Closed
recidive opened this issue Aug 18, 2015 · 10 comments
Closed

Refactor lib/* in ES6? #2426

recidive opened this issue Aug 18, 2015 · 10 comments
Labels
question Issues that look for answers.

Comments

@recidive
Copy link

Hello folks!

Do you have any plans on refactoring core library stuff to ES6?

It also lacks proper code documentation.

Is such refactor welcome as contribution?

Thanks!

@mscdex mscdex added the question Issues that look for answers. label Aug 18, 2015
@Qard
Copy link
Member

Qard commented Aug 18, 2015

There are already ES6 features being used in many places. We're just converting gradually, as particular areas are touched. Wholesale refactoring is probably not a great idea as it tends to introduce high risk of merge conflicts.

Also, I believe such a refactor would be considered a style change, which we tend to not accept on their own.

@bnoordhuis
Copy link
Member

It also lacks proper code documentation.

If you mean jsdoc or doxygen-style documentation, we don't do that. The canonical API documentation is in doc/api.

@silverwind
Copy link
Contributor

I see two additional issues:

  • ES6 code isn't yet optimized as well as older variants. There will most likely be a performance hit.
  • As @Qard mentioned, in the end it's just a style change which will make backtracking changes through git blame harder.

@targos
Copy link
Member

targos commented Aug 18, 2015

And let's not forget that the code is technically already valid ES6. We don't need to use all the new features to say it.

@recidive
Copy link
Author

This is not just about style, the whole thing about ES6 is the possibility to produce better JS code, less verbose and more maintainable. And no better place to encourage best coding practices than in node core library itself.

I believe innovation is driven by changing and breaking things, not by maintaining old code for backwards compatibility or by worrying about performance issues without benchmarks.

This is surely not a thing to be done in the current release cycle, but what about the next? Are we going to maintain all those legacy code forever?

@mikeal
Copy link
Contributor

mikeal commented Aug 18, 2015

@recidive the problem here is that node's core libraries are in the hot path of every node.js application in production. We can't go refactoring code in a way that could be slower or inadvertently change functionality for reasons this reckless.

New features are great for application developers but in the hot code paths we have to rely on code style that can be highly optimized by the VM which usually means staying away from new features which have not been around long enough to be optimized.

That said, if you have a patch that uses a bunch of string templates, or some other new feature, and it turns out to benchmark faster, it's likely to make it in.

@Qard
Copy link
Member

Qard commented Aug 18, 2015

Also, don't be discouraged by the push back here. It's not that we don't want to use these ES6 features more. It's just that we want changes to happen incrementally, so they can be carefully reviewed and more easily merged. Some changes might be fine while others are not, and grouping them together might just get them all rejected together.

Some low-impact features that can be used currently are const, template strings and symbols. There's probably others too. I'm not sure what our policy is on let, but I'd really like to start using it.

Larger features like promises, generators and classes have a somewhat larger change surface or performance profile, and are therefore being avoided for now.

@Trott
Copy link
Member

Trott commented Aug 18, 2015

Some ES6 features that might be looked upon with skepticism in lib/* would likely be met with less resistance in test/*. For example, I used promises writing one test. (In that particular case, it made the code more readable, in my opinion.)

And there seems to be some interest in speeding up tests lately. (See #2410, #2393, and #2429.) So if you're into ES6 and benchmarks, a refactor of any tests that achieve better performance with ES6 features would be great to see.

@q2dg
Copy link

q2dg commented Aug 21, 2015

Maybe https://github.com/nodejs/NG could be a good place to discuss it?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 21, 2015

Closing, as this has been explained pretty well. ES6 is making it into lib, slowly but surely.

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

No branches or pull requests

10 participants