google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.41k stars 1.15k forks source link

Add Support for Public Class Fields #2731

Open davydotcom opened 6 years ago

davydotcom commented 6 years ago

It is becoming common place to use class fields and this is not yet supported from ES6 to ES5 conversion via closure compiler

https://github.com/tc39/proposal-class-fields

rahbari commented 6 years ago

Also Static Class Fields: https://github.com/tc39/proposal-static-class-features/

teppeis commented 6 years ago

They are stage 3 features now. https://github.com/tc39/proposals

rahbari commented 6 years ago

I wish to see this feature soon in closure compiler, having static fields helps a lot in class feature encapsulation.

class A {
    static a = 1;
}

instead of:

class A {}
A.a = 1;
brad4d commented 6 years ago

We've been looking forward to this feature making it to stage 4, also. We're not ready to implement it in closure-compiler yet, but it is on our minds.

justinfagnani commented 6 years ago

@brad4d does a feature have to make it to stage 4 before implementing? Browsers will generally implement before stage 4, and all the other compilers I know of support class fields.

ChadKillingsworth commented 6 years ago

We've implemented features at stage 3 before. It's just a matter of priority or someone willing to contribute the work.

arv commented 5 years ago

At stage 3 now and shipping in V8.

https://github.com/tc39/proposal-class-fields https://twitter.com/_gsathya/status/1100845457876541442

blickly commented 5 years ago

Exciting!

Created internal issue: http://b/127859638

brad4d commented 5 years ago

@justinfagnani I'm actively working on an official SLO for handling new features. It still requires review, but the current draft calls for us to start working on stage 3 features when we see that browsers have started adding support. (Lots of definition needed of what that means exactly.)

@arv One thing that I find discourages me from prioritizing class fields over other stage 3 features, like BigInt, is that there still are no official tests associated with the proposal according to the TC39 active proposals page. http://go/gh/tc39/proposals/blob/master/README.md

We would like to have these to base our unit tests on. Do you know why there aren't any yet? Thanks.

justinfagnani commented 5 years ago

@brad4d there are a ton of tests for public, private, and static fields and methods: https://github.com/tc39/test262/tree/master/src/class-elements

brad4d commented 5 years ago

@justinfagnani Thanks for pointing out those tests. I guess the page I pointed to needs to be updated.

loadedsith commented 5 years ago

If/when this ships, will it require an opt-in? Perhaps via @language ECMASCRIPT_2017?

blickly commented 5 years ago

The proposal is still a stage 3 proposal, i.e. not yet part of a language standard, but the current assumption (I think) is that it will become part of ES2020

justinfagnani commented 5 years ago

@blickly Stage 3 is when implementors should implement. Chrome is already shipping public and private class fields. As an implementor, the time for Closure to implement is now, you don't have to wait for Stage 4.

blickly commented 5 years ago

Agreed, I'm just responding to the question of which language version it will be in (i.e. not ES2017)

justinfagnani commented 5 years ago

Ah, sorry then!

brad4d commented 5 years ago

@loadedsith if/when this feature ships you'll at least initially have to ask the compiler to handle it by specifying language_in to be the ES version in which it lands. (Probably ES_2020 as @blickly pointed out.)

When we feel it is safe to do so, we'll likely modify the default for language_in to be that new ES version, at which point it is no longer an "opt in" feature.

loadedsith commented 5 years ago

Thank you 3 for the informative replies. I'm looking forward to it.

robpalme commented 4 years ago

Hello @brad4d - I am very interested in the SLO you mentioned. Is there a link to this?

The Class Fields proposal has shipped in Chrome, Node LTS, Moddable XS, QuickJS, Babel, TypeScript nightly, and Firefox (public-fields-only). The corresponding test262 tests were declared complete on 10 November 2017. Do you think it is now appropriate to ship in GCC?

brad4d commented 4 years ago

I'm afraid the SLO isn't in a publicly accessible place right now, but the gist of it is we want to have support for new JS features added to the compiler and available with --language_in=ES_20XX within 6 months of ES_20XX being officially defined.

Although they've been shipped in some form or other by several products, the TC39 committee hasn't discussed moving the relevant proposals for class fields forward to stage 4 (i.e. officially in the next version of the language) since January 2019.

See https://github.com/tc39/proposals/blob/master/README.md

It's possible that they will promote them to stage 4 in the February meeting, making them part of ES_2020, but we have other things that are definitely in ES_2020 to work on first.

We're currently working on nullish coalescing (https://github.com/tc39/proposal-nullish-coalescing) and optional chaining (https://github.com/tc39/proposal-optional-chaining), which were promoted to stage 4 in December.

justinfagnani commented 4 years ago

Waiting for stage 4 is really the wrong thing for tools like this. Stage 3 is when tools are expected to implement. Stage 4 is recognition that there wasn't some catastrophic failure in stage 3. Shipping is what counts, and class fields have shipped in enough implementations to not be at risk of rollback, (even with previously threatened TC39 consensus shenanigans, which aren't even a potential issue anymore).

Note the wording for stage 3:

The solution is complete and no further work is possible without implementation experience, significant usage and external feedback.

It sounds like the SLO / feature approach, if strictly applied to stage 4 features only, would leave out very widely implemented things like dynamic import(). I know there's not a readily available other label for a feature set, but the official ES year isn't great either. It doesn't indicate what's actually usable in any implementation.

ctjlewis commented 4 years ago

The fact that V8 has supported this for over a year yet Closure Compiler will immediately throw an error seems a little silly. Support for this would be completely non-breaking, there would be no difference in compilation output for people who did not use static public fields in their source code. Waiting for Stage 4 literally just prevents people from using this pattern at all in code that will be CC'd.

I'm having to do:

class Child extends Parent {
    static get a() {
       return 4;
   }
}

To override a static property right now, if I want my code to compile without doing a class declaration and then setting the field manually (which is horribly disorganized). Replacing with static a = 4 works in Chrome without issue, but the compiler won't take it at all.

Would a PR adding support for static class fields get approved?

brad4d commented 4 years ago

Adding a new feature like this to the compiler takes a large number of changes. There's no way to do it in a single PR.

As a sampling:

And there's more....

Also, class fields behavior as defined by the spec is more complex than one would initially think. Doing this work requires creating a plan, getting agreement that it's a good one, then working through it with regular discussions of problems as they come up.

Sorry. If it were easy, we would have done it already.

This feature is top of my mind when it comes to things to work on after our current efforts to support BigInt and optional chaining.

ctjlewis commented 4 years ago

Thanks for the clarification @brad4d.

ctjlewis commented 4 years ago

I've resorted to transpiling class fields to get methods until native support for this is added.

d3x0r commented 3 years ago

adding private fields and methods too?

runthis commented 3 years ago

plzsir my #private fields

cristhianDt commented 3 years ago

Hi

Do you have contemplate include Private class fields soonly ?

Currently I get this error:

`ERROR - [JSC_PARSE_ERROR] Parse error. '}' expected

cOps

^

1 error(s), 0 warning(s)`

thanks

blickly commented 3 years ago

@helenlpang is currently working on support for public fields (both static and non), but #private fields will unfortunately have to wait until after that is done.

rconnamacher commented 2 years ago

Just a note, Class Fields are now Stage 4 and included in the ECMAScript 2022 draft specification. https://tc39.es/ecma262/#sec-classfielddefinition-record-specification-type

roman01la commented 2 years ago

@brad4d Is there any status updates or ETA on this ticket?

brad4d commented 2 years ago

A significant amount of work has done on supporting public class fields, but it isn't finished yet. ETA for that is likely Q2 2023

private fields are a much harder problem, and I'm not sure when that will be available.

roman01la commented 2 years ago

Thanks for the update!

d3x0r commented 2 years ago

It would be nice if there was even just non-transpile option for private fields and methods; like for minimization only which shouldn't change the structure, just identifiers. Also I'd bet most transpiling to an older ES might not require that they behave exactly like private fields (other than being non-enumerable).

roman01la commented 2 years ago

The above about "not require that they behave exactly like..." spikes a question: @brad4d out of curiosity and to understand how Closure is being developed, as a user of Closure I'm also comparing it to other transpilers that exist in JS ecosystem, to my knowledge those tools were transpiling public and private fields for quite some time already. Thus my question is: what does it make so time consuming for Closure to implement that? Does it mean that other tools have 80% correct implementation and Closure is trying to it get 99..100% correct? Or is it something else?

robpalme commented 1 year ago

Hello @brad4d, please consider this the lightest and hopefully politest reminder in the world.

we want to have support for new JS features added to the compiler and available with --language_in=ES_20XX within 6 months of ES_20XX being officially defined.

Class Fields shipped in Chrome 74 in April 2019. They went to Stage 4 in in TC39 in April 2021. ES2022 scope was defined by TC39 in Feb 2022. Ecma approved ES2022 in June 2022 (8 months ago). So maybe that now qualifies the feature to be prioritized.

Thank you for all your hard work!

brad4d commented 1 year ago

Very polite, @robpalme thanks.

Getting this support completed is top of my mind for the coming quarter. I can't promise it will be done, but I do expect to put work into it.

We worked on this last summer, but hit some difficulties with appropriate scoping for the initializers and didn't finish. And there are always other fires burning...

bannostookaylo commented 1 year ago

Was hoping private class members would have been added by now... Curious where the status of this is at right now... I cam put the ugly _ in front of my private vars but i don't want to lol

Kaspazza commented 1 year ago

Hope this will be updated, as I can't import newer versions of some js libraries (like marked.js) due to that...

dswitzer commented 1 year ago

Just went to add some new code to our deployment pipeline that using private class variables and ran into this issue. So count me as 👍 for needing this feature!

thheller commented 10 months ago

Any update on this? There is an ever increasing amount of npm packages using private/static class fields and that makes them unusable from shadow-cljs (a widely used ClojureScript build tool).

brad4d commented 10 months ago

We are trying to add support for class public fields right now, but have had some implementation problems. Still we expect to have that support done by the end of 2024, hopefully sooner.

However, fair warning, it will be restricted to:

  1. Only public fields. Private fields (the kind that start with #) will still not be supported for awhile, because they're a lot more trouble.
  2. The transpilation will not be fully spec compliant. Specifically, it will use simple assignment semantics (this.field = value;), not define semantics (Object.defineProperty(this, 'field', ...);), because define semantics are much too verbose.

Regarding #2:

The behavior difference is meaningful when a class, B, extends class, A, and both declare the same field name, f, but using different semantics. A declares f as a normal field while B declares it using get/set methods. Or vice versa.

In order to make sure the JS code we are compiling will behave the same way whether the class fields are transpiled or not, we intend to simply forbid all such usages.

The compiler will report errors whenever it can tell that the code it is compiling would behave differently depending on whether assignment or define semantics are used for initializing class fields.

This isn't a significant loss of capability for us, because we also prefer to discourage use of getters and setters whenever we can, because they make code much harder to optimize.

rictic commented 10 months ago

The transpilation will not be fully spec compliant. Specifically, it will use simple assignment semantics (this.field = value;), not define semantics (Object.defineProperty(this, 'field', ...);), because define semantics are much too verbose.

Have you thought about how this will interact with decorators, which typically require the accessor keyword and thus create a getter/setter pair?

brad4d commented 10 months ago

We have not. Thanks for pointing that out.

thheller commented 9 months ago

Only public fields. Private fields (the kind that start with #) will still not be supported for awhile, because they're a lot more trouble.

This is kind of a problem for us. Unfortunately the broader JS world has already adopted these quite quickly, and that only keeps growing. ClojureScript itself doesn't care, but already quite a few popular npm packages that used to work no longer do.

Would it be possible to have the compiler just keep the private fields as-is without any support for transpilation or "polyfilling"? I don't care much about checking or enforcing specs either, fine to just treat them as public. Maybe just renaming #foo to __priv_foo?

Telling someone to use a different tool is a rather unsatisfying answer to give as a tool author.

jespertheend commented 9 months ago

fine to just treat them as public. Maybe just renaming #foo to __priv_foo?

One problem with this is that classes can extend each other and reuse existing properties. For instance:

class Foo {
    #x = true;
    getMine() {
        return this.#x;
    }
}

class Bar extends Foo {
    #x = false;
}

console.log(new Bar().getMine())

This code logs true, but if you were to make the fields public, it would log false.

That said, I wouldn't mind support without any transpilation or polyfilling either. Private class fields have been supported in major browsers for two years now and I don't need to support older browsers.

thheller commented 9 months ago

One problem with this is that classes can extend each other and reuse existing properties.

Could just use a class specific prefix. __priv_Foo_x and __priv_Bar_x, or assign some other kind of unique prefix per class. As far as I understand it the private field is only valid inside the class definition, which makes the impact rather controllable AFAICT.

Kaspazza commented 9 months ago

@brad4d, I fully agree with @thheller on this. It's better to be able to use the library with the newest functionalities and miss on some developer features/checks, than to be completely blocked.

justinfagnani commented 9 months ago

@brad4d

  1. Only public fields. Private fields (the kind that start with #) will still not be supported for awhile, because they're a lot more trouble.

I think the transform to a WeakMap, which maybe a bit verbose, isn't overly complicated. Is the concern optimization complexity? I know that many people are using Closure after a first pass with TypeScript, in which case Closure will be seeing the downleveled WeakMaps...

  1. The transpilation will not be fully spec compliant. Specifically, it will use simple assignment semantics (this.field = value;), not define semantics (Object.defineProperty(this, 'field', ...);), because define semantics are much too verbose.

What's the plan for moving to define semantics in the future? Given very good browser support for class fields (> 95% by usage), many people may want leave class fields in the output of one build, and have a separate build that compiles out the fields. That separate build could be larger for the small minority of users that have to use it.

@rictic

Have you thought about how this will interact with decorators, which typically require the accessor keyword and thus create a getter/setter pair?

I think the proposed restriction actually works pretty well for this case in practice.

brad4d commented 9 months ago

@brad4d

  1. Only public fields. Private fields (the kind that start with #) will still not be supported for awhile, because they're a lot more trouble.

I think the transform to a WeakMap, which maybe a bit verbose, isn't overly complicated. Is the concern optimization complexity? I know that many people are using Closure after a first pass with TypeScript, in which case Closure will be seeing the downleveled WeakMaps...

The downleveled WeakMaps are a definite problem for optimization. They're too big and introduce a level of indirection that closure-compiler cannot easily reason about for unused code removal and property renaming.

When we do add support for transpiling #-private properties we expect to enforce their privacy at compile time but, if transpiling, convert them to guaranteed-unique property names.

  1. The transpilation will not be fully spec compliant. Specifically, it will use simple assignment semantics (this.field = value;), not define semantics (Object.defineProperty(this, 'field', ...);), because define semantics are much too verbose.

What's the plan for moving to define semantics in the future? Given very good browser support for class fields (> 95% by usage), many people may want leave class fields in the output of one build, and have a separate build that compiles out the fields. That separate build could be larger for the small minority of users that have to use it.

The plan is to never allow any code within Google that depends on the define semantics until an unknown future point when we never have to transpile class fields anymore.

Because Google's products generally try to support the whole world, we sadly have to lag very far behind. We expect there will be a very long period of time where we serve un-transpiled fields to some clients but transpiled fields to others. We need to ensure that the behavior is the same in both cases and also that the transpiled code size is still acceptable.