Skip to content
This repository has been archived by the owner on Dec 3, 2019. It is now read-only.

ES6 feature introduction: classes #2176

Closed
eakuefner opened this issue Mar 24, 2016 · 8 comments
Closed

ES6 feature introduction: classes #2176

eakuefner opened this issue Mar 24, 2016 · 8 comments
Assignees
Labels
Milestone

Comments

@eakuefner
Copy link
Contributor

Since @zeptonaut has initiated a procedure for opening up the tracing codebase to new ES6 features, we want to try classes next.

I'm doing some refactoring on Numeric right now, and it occurred to me that this would probably be a good place to start, since numerics are used by a fair amount of unit tested code, but not in so many places that any refactoring that has to be done would be insurmountable.

@eakuefner eakuefner self-assigned this Mar 24, 2016
@eakuefner eakuefner added the ES6+ label Mar 24, 2016
@eakuefner eakuefner added this to the M51 milestone Mar 24, 2016
@zeptonaut
Copy link
Contributor

Awesome! Thanks so much for doing this Ethan! I'd love to hear more as progress is made.

@zeptonaut
Copy link
Contributor

Hey Ethan, any update on when you might be able to try this out?

@eakuefner
Copy link
Contributor Author

It turns out that Numeric is not really a good place to start because it's heavily in flux right now.

I'm happy to push on this but we should find another place to do it -- I think you had another small class in mind?

@zeptonaut
Copy link
Contributor

(We talked about this offline, and I suggested either Event or PowerSample - preferably Event because it's so core, as long as it works from Ethan's perspective.)

@eakuefner
Copy link
Contributor Author

I'm working on a big patch to convert the Event hierarchy to classes. I don't think we need to exercise the same caution that we did for arrow functions in terms of "stuff blowing up", so I'm fine with this being a big patch, especially since people work with Events so often that making it a class should be a nice usability improvement.

@eakuefner eakuefner modified the milestones: M54, M51 Aug 25, 2016
@eakuefner
Copy link
Contributor Author

Revisiting this, folks are already using classes; I think we should turn them on in the style guide.

chromium-infra-bot pushed a commit that referenced this issue Aug 25, 2016
Folks are already using ES6 classes in tracing and everything seems to work
fine. Let's approve them in the style guide.

BUG=catapult:#2176

Review-Url: https://codereview.chromium.org/2282563002
@petrcermak
Copy link
Contributor

Should we convert all classes to use proper classes?

@benshayden
Copy link
Contributor

Converting at least the Event hierarchy would fix the prototype.constructor link, which would vastly simplify EventRegistry as discussed in https://codereview.chromium.org/2242733002/

grep says there are 499 instances of '.prototype = '. So that's a lot of typing. But there are only a dozen or two classes in the Event hierarchy, iirc.

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

No branches or pull requests

4 participants