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

Why are TS get/set definedProperties in a class transpiled as enumerable/writable? #3610

Closed
wardbell opened this issue Jun 24, 2015 · 14 comments · Fixed by #32264
Closed

Why are TS get/set definedProperties in a class transpiled as enumerable/writable? #3610

wardbell opened this issue Jun 24, 2015 · 14 comments · Fixed by #32264
Labels
Breaking Change Would introduce errors in existing code Committed The team has roadmapped this issue ES6 Relates to the ES6 Spec Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@wardbell
Copy link

I just confirmed in the TS Playground that the following TS:

class Foo {
    get bar() { return true;}
}

Produces this JS:

var Foo = (function () {
    function Foo() {
    }
    Object.defineProperty(Foo.prototype, "bar", {
        get: function () { return true; },
        enumerable: true,
        configurable: true
    });
    return Foo;
})();

This MDN link confirms that, for Object.definedProperty accessors:
• enumerable: Defaults to false.
• configurable: Defaults to false.

Clearly the generated code is overriding the default values for these keys.

Plowing through the ES6 spec is not something I do. So I’m not sure that ES6 hasn’t changed the defaults. But I’d be surprised.

Now one of the things I miss in TS/ES6 class definitions is the ability to control these keys. It is also not easy to embellish a class with defined properties later … properties that might have non-default definitions

But that’s not my point or my question … which is, why is TS deviating from the defaults?

I get why I’d want the property to be enumerable most of the time – strange that the spec defaults to false. I don’t understand why I should want it to be configurable (maybe because I don't like something about the property TS generated for me?).

In any case, I don’t get how TS (or ES6) gets away with changing the ES5 defaults.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 24, 2015

This might actually be a bug.

14.5.14 Runtime Semantics: ClassDefinitionEvaluation

With parameter className.

    ClassTail : ClassHeritageopt { ClassBodyopt }

[...]
For each ClassElement m in order from methods

  1. If IsStatic of m is false, then
  2. Let status be the result of performing PropertyDefinitionEvaluation for m with arguments proto and false.
  3. Else,
  4. Let status be the result of performing PropertyDefinitionEvaluation for m with arguments F and false.

That second parameter to PropertyDefinitionEvaluation is enumerable.

Though, PropertyDefinitionEvaluation sets [[Enumerable]] to true, so the current behavior may be correct in that respect. (referenced wrong section)

This part of the spec is hard to trace and it would be good to have more eyes on this if possible.

@DanielRosenwasser DanielRosenwasser added the Needs More Info The issue still hasn't been fully clarified label Jun 24, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Jun 26, 2015

/CC: @bterlson

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 11, 2015
@gilamran
Copy link

Sorry for polluting this discussion, but why isn't the result code just use ES5 getters/setters ? Why use defineProperty?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2015

@gilamran you want to put the instance getters/setters on the prototype of the constructor function, and the static ones on the function itself. there is no other way to create something that is both a function, and set properties on it. you can do this with object literals, but then they are not functions.

@gilamran
Copy link

@mhegazy Got it, thanks!

@bterlson
Copy link
Member

Sorry I'm late to this thread...

Class methods and getters/setters are created with enumerable false, configurable true, writable true. Seems like an ES6 conformance bug.

(The rationale here was in part to align classes with the built-in types, eg. consider that Array.prototype.* are non-enumerable).

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript and removed Needs More Info The issue still hasn't been fully clarified Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 28, 2015
@DanielRosenwasser
Copy link
Member

Thanks for clarifying @bterlson, and thanks for noticing @wardbell.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus ES6 Relates to the ES6 Spec and removed Bug A bug in TypeScript labels Aug 28, 2015
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Nov 3, 2015
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 1.8 milestone Nov 3, 2015
@RyanCavanaugh RyanCavanaugh added the Breaking Change Would introduce errors in existing code label Nov 3, 2015
@RyanCavanaugh
Copy link
Member

We should change our emit to align with the ES6 semantics. This will be a breaking change (probably a large one, relatively speaking), but is required for ES5/ES6 compat.

@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.0 Jun 6, 2016
@calebboyd
Copy link

I believe I've run into this issue. Here is a repeatable snippet that works as intended when targeting es6 or using Babel.
link

@mhegazy mhegazy modified the milestones: TypeScript 2.1, Future Sep 21, 2016
@vvakame
Copy link
Contributor

vvakame commented Nov 23, 2016

Is anyone doing this work?
I met this issue in SkateJS.
SkateJS wrote by Babel. SkateJS expected inherit static member of enumerable: false.
But TypeScript can't inherit it.

@Arlen22
Copy link

Arlen22 commented Sep 7, 2017

Any updates on this?

@coldino
Copy link

coldino commented Mar 24, 2018

I see this is still an issue in 2018.

This is clearly a bug. There is an obvious functional difference in the code generated when targeting es5 and when targeting es6.

I'm working with code that depends on properties being non-enumerable, so TypeScript targeting es5 simply produces broken code. I believe relying on properties being non-enumerable is a reasonable thing to do, especially given this is the behaviour of all current es6 browsers.

Please consider fixing this. If there is too much impact on existing code then an option to control the output variant would be useful and allow a graceful change-over period.

@RyanCavanaugh
Copy link
Member

image

@michkot
Copy link

michkot commented Feb 26, 2021

This was closed with #32264, but this issue description is ambiguous/misleading, as it makes references to Object.definedProperty defaults and "breaking change" of defaults between ES5 and ES6. I would like to provide clarification for potential future readers.


The real problem (as hinted by #3610 (comment) later in the discussion) was that there was already get()/set(val) defined for object literals at ES5 (with enumerable: true, configurable: true) (see
https://262.ecma-international.org/5.1/#sec-11.1.5), but when ES6 and classes was introduced, get() and set(val) in ES6 classes got different semantics (enumerable: false, but still configurable: true) (see, https://262.ecma-international.org/#sec-method-definitions-runtime-semantics-propertydefinitionevaluation).
However, TypeScript's get() and set() in classes had the ES5 transpilation semantics taken from object literals, creating a discrepancy between semantics of get()/set(val) in classes when the target was ES5 (object-literal-like created manually using Object.defineProperty) and when it was ES6+ (ES6-native with get/set keywords).

For anyone wondering about the semantics of Object.defineProperty, that itself DID NOT change between ES5 and ES6- see https://www.ecma-international.org/wp-content/uploads/ECMA-262_5th_edition_december_2009.pdf (ES 5, section 8.12.9);
https://262.ecma-international.org/5.1/#sec-8.12.9 (ES 5.1) also states that all fields of 'Property Descriptor' are by default false.

If a field is absent then its value is considered to be false.


I would like to note that as a greenhorn JS/TS programmer, I did not realize that there actually was support for get/set in object literals and that it might have different semantics then get/set in class definitions.

  • TS (at least for v >=1.8 && < 3.9) had the aforementioned "enumerable: true" semantics for class accessors, as described in http://javascript.xgqfrms.xyz/pdfs/TypeScript%20Language%20Specification.pdf TS 1.8, sec 8.7.1, page 134.
  • Since there is no updated standard document for TS, the only official documentation is https://www.typescriptlang.org/docs/handbook/classes.html#accessors - but that completely omits the important technical detail of what descriptors will get accessors get - I think it might be nice to add it there so people don't have to comb true JS standard to find it out.
  • it might also be nice to warn novice users of "keyof" / TypeScript in general that even though property is part of a type (public in class), it says nothing about the property being enumerable or configurable.

Finally, I'd like to add an (opinionated) highlight of important facts from developers PoV when using JS/TS classes, since I had to think about all this when trying to decipher this thread :

  • there is (in JS/TS) a "syntactic sugar" for creating accessors in object literals and classes, it will internally boil down to an equivalent of Object.defineProperty with AccessorDescriptor
  • it is inconsistent between object literals and classes
  • the syntactic sugar (get/set) lacks expressive power for developer to be able explicitly state what Property Descriptors are to be used (if not counting decorators, that come with a strong warning "!experimental!" https://www.typescriptlang.org/docs/handbook/decorators.html )
  • moreover, there is a related change to how class fields are handled in TS https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier, which is again caused by JS going in other direction than TS choose to use in the past;
  • even though [[Define]] semantics for fields (enabled with "useDefineForClassFields" ) makes it possible to enforce "readonly" via explicitly defining the field/property with writable: false, TS does not use that opportunity (it would be nice option! - maybe should submit a new issue for that, but it would produce different runtime behavior)

What would be my conclusion for writing classes in JS/TS, taking in account ESNext:

  • do not use get/set definitions if you expect to mix object literals and class definitions using them in single codebase
  • do not use get/set definitions when you usually need non-default enumerable (or configurable) "settings" (rather just declare it like a field and use something like https://fettblog.eu/typescript-assertion-signatures/ to safely call Object.defineProperty)
  • do not define (ugh, why is it called declaration in the JS proposal, when it's actually a definition?) (public) fields on classes unless you expect the "defineProperty call in constructor"-like behaviour to happen!
  • do not define readonly public fields on classes if you want the "non-writability" to be enforced at runtime -> just declare the property and use something like https://fettblog.eu/typescript-assertion-signatures/
    • there is no writeonly typescript keyword or JSDoc annotation, and you can not declare an accessor.. so the only thing you can do is to add field-level comment stating that the property is not writable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Committed The team has roadmapped this issue ES6 Relates to the ES6 Spec Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.