Skip to content

preset class properties to undefined #12437

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

Closed
Jordan-Hall opened this issue Nov 22, 2016 · 31 comments
Closed

preset class properties to undefined #12437

Jordan-Hall opened this issue Nov 22, 2016 · 31 comments
Labels
Breaking Change Would introduce errors in existing code ES Next New featurers for ECMAScript (a.k.a. ESNext) Suggestion An idea for TypeScript Waiting for TC39 Unactionable until TC39 reaches some conclusion

Comments

@Jordan-Hall
Copy link

TypeScript Version: 2.1.1

Code

class PartialPolicySearchIten {
	public displayValue: string;
	public oriNumber: string;
	public parentName: string;
	public sysNumber: string;
	public systemCode: number;
	public systemName: string;
}

Expected behavior:

class PartialPolicySearchIten {
       constructor() {
        this.displayValue = undefined;
	this.oriNumber = undefined;
	this.parentName = undefined;
	this.sysNumber = undefined;
	this.systemCode = undefined;
	this. systemName = undefined;
       }
}

Actual behavior:
class PartialPolicySearchIten { }

The problem comes when using Reflection it can't find the properties. I would expected that the properties exists as i'm redefining them. Otherwise their very little point is declaring properties of a class.

@Jameskmonger
Copy link

Jameskmonger commented Nov 22, 2016

You need to explicitly initialise them as undefined to have the effect you want.

Input

class ImplicitlyUndefinedProperties {
	public foo: string;
	public bar: string;
}

class ExplicitlyUndefinedProperties {
	public foo: string = undefined;
	public bar: string = undefined;
}

Output

var ImplicitlyUndefinedProperties = (function () {
    function ImplicitlyUndefinedProperties() {
    }
    return ImplicitlyUndefinedProperties;
}());
var ExplicitlyUndefinedProperties = (function () {
    function ExplicitlyUndefinedProperties() {
        this.foo = undefined;
        this.bar = undefined;
    }
    return ExplicitlyUndefinedProperties;
}());

Therefore I think that this is not an issue but is intended behaviour.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript ES Next New featurers for ECMAScript (a.k.a. ESNext) labels Nov 22, 2016
@DanielRosenwasser
Copy link
Member

While this is the original/current behavior, the current ES proposal seems to actually imply that a property will indeed be defined & set to undefined even without an initializer.

https://tc39.github.io/proposal-class-public-fields/#initialize-public-static-fields

@RyanCavanaugh
Copy link
Member

The proposal is now Stage 3 with this behavior. We need to discuss whether or not to take a break for this

@buschtoens
Copy link

buschtoens commented Dec 3, 2018

(Public) class fields are enabled by default in Chrome 72 with the initializer semantics outlined above. This part of the spec is extremely unlikely to change. Current topics of debate are just how private fields should work AFAICT.

Can we please revisit and get a decision here? While the Babel transform is not officially supported, they would want to wait for a decision by the TypeScript team before implementing any fixes: babel/babel#9105

The major arguments for keeping the current semantics to me are:

  • no breakage
  • "re-typing" a class field in a derived class is easier.\

In favor of switching:

  • spec compliancy
  • less cognitive load for JS developers migrating to TS

Any input would be much appreciated ☺️❤️

@nicolo-ribaudo
Copy link

  • "re-typing" a class field in a derived class is easier.\

For type-only information something like this could work:

class Foo {
  public x: string; // initialized to undefined
  declare public y: string; // type-only
}

@kutomer
Copy link

kutomer commented Dec 4, 2018

@nicolo-ribaudo not sure how it's possible, tried it on ts playground and got declare' modifier cannot appear on a class element.

@buschtoens
Copy link

@kutomer I think what @nicolo-ribaudo wanted to express is that the declare keyword could be used as new syntax, i.e. it does not work yet and would be a breaking change to TypeScript. 🙂

@nicolo-ribaudo
Copy link

Yeah exactly

@chriskrycho
Copy link

Also worth noting here that moving to spec compliance is a loud breaking change, because the existing types end up being wrong with strictNullChecks: true:

class Foo {
  public x: string; // initialized to undefined
}

☝️ would be wrong; the type would need to be:

class Foo {
  public x?: string; // initialized to undefined
}

This will be a non-trivial breaking change to a lot of codebases. (It intersects with definite strictPropertyInitialization a bit as well.)

@vbraun
Copy link

vbraun commented Jan 5, 2019

Case in point, this breaks angularjs (tested with 1.5.11) bindings to components:

class Controller {
     public foo: () => string;
}

const component: ng.IComponentOptions = {
     controller: Controller,
     controllerAs: '$ctrl',
     bindings: {
        foo: '&',
    }
}

Works with tsc (angularjs initializes $ctrl.foo) but fails with @babel/typescript ($ctrl.foo is initialized as undefined and angularjs does not overwrite it).

@trotyl
Copy link

trotyl commented Jan 5, 2019

@vbraun It's an AngularJS bug already fixed in 1.6.

Previously AngularJS sets binding properties even before the constructor being invoked, making it totally incompatible with ES class (only work when compiled to ES5, which doesn't really keep class semantics).

In 1.5, you'll need to manually set the option to false, as patch version is considered non-breaking.

@brianmhunt
Copy link

I suggested the own keyword in ^^^, which would use the new (set-property-to-undefined) behaviour. Might not work here, but thought to mention it.

@davidje13
Copy link

@chriskrycho this doesn't have to be a breaking change. Right now the following code is considered valid (unless strictPropertyInitialization, or more generally strict, is true), even if strictNullChecks is true:

class Foo {
  public x: string;

  public getX(): string {
    return x;
  }
}

But this can clearly cause runtime issues because getX will return undefined. It is no worse if x is declared and assigned to undefined automatically, since this is (approximately) the existing behaviour, with the same possible bugs.

I suggest declaring the property and setting it to undefined if not specified in a constructor (to match the spec), but introduce no new warnings or errors. The existing strict checks are already sufficient, and anybody who has those turned off is already facing the same possible runtime bugs.

@chriskrycho
Copy link

You’re partly correct, but there are ways that people with strictNullChecks but not strict or strictPropertyInitialization can be broken by this, precisely it’s breaking in terms of compiler output, not only in terms of type-checking. If you have a way that a property is known to be initialized outside the constructor (e.g. component arguments, which may be checked via debug assertions or the like), the behavior can change with this. We saw these breaking changes in practice across many applications when we changed to using TS only for type checking and Babel (which is spec compliant) for actual compilation in Ember apps and addons.

That said, deferring it to strictPropertyInitialization or strict doesn’t seem unreasonable, since it’s impossible for this not to be a breaking change in some way. (It should, however, be code-mod-able given a satisfactory design.)

@ajakaja
Copy link

ajakaja commented Sep 13, 2019

Could I propose that only fields whose types include undefined be initialized as such? It seems to me that initial example, where the fields were just removed, ought to be disallowed -- if the value is non-optional but also never set in a constructor, then that should just be a type error.

So this should be fine:

class Foo {
  public x?: string; // initialized to undefined
}

and this should be an error:

class Foo {
  public x: string; // TypeError -- x can't be undefined
}

@leandro-hl
Copy link

Any news on this? This kind of behavior is as important as common and standard. Would be great to fix it.

@IgorSzymanski
Copy link

IgorSzymanski commented Jun 7, 2020

What is the use case for this one?
undefined is the spiritual equivalent of the lack of a property, thus:

type LolRly = { prop?: bool }
const A: LolRly = {}
const B: LolRly = { prop: undefined }
console.log(typeof A.prop) // undefined
console.log(typeof B.prop) // undefined
console.log(JSON.stringify(A)) // {}
console.log(JSON.stringify(B)) // {}

If you want to do some dirty, 5-minute life hacks with iterating over an object properties (and I can smell from a mile, that it's exactly what you're trying to do), then null is for that to represent an existing property with en empty (NULLABLE) value.
Even if JS/TS can produce and allows the existence of an object with a property initialised with undefined value, it should be treated as if it wasn't there in the first place.

@ljharb
Copy link
Contributor

ljharb commented Jun 8, 2020

@IgorSzymanski the difference, however, is observable, and it's important for it to be correct.

@IgorSzymanski
Copy link

IgorSzymanski commented Jun 10, 2020

@ljharb I ask again for the use case.
Also, if you do not initialise and optional property, why do you expect it to be initialised with undefined? If you do not initialise an optional property, the expected behaviour is it's not there, uninitialised shouldn't be initialised, and that's exactly what happens.

Also, the OP example is horrible.

If you define a property as non-optional number, it should be nothing else, but a number. No part of the code says it can be undefined.

@ljharb
Copy link
Contributor

ljharb commented Jun 10, 2020

Anything using object spread or Object.assign will care about it being a present, own property, even if set to undefined. undefined is not simply "not defined", in JS, there exists "absent", "present but set to undefined", and "present but set to non-undefined".

@IgorSzymanski
Copy link

IgorSzymanski commented Jun 10, 2020

Then initialise your properties explicitly. This is a breaking change, it breaks the logic behind explicit property initialisation and, once again, I do not see any use case where it should matter.
In OOP, you shouldn't really ask for properties of a class instance, because it breaks its encapsulation.
In FP, you wouldn't even use a class for this, and therefore you would have to initialise the property on your own.
This change is nothing but an attempt to encourage a bad programming style.

I also do not see, why you would want to run a class instance against Object.assign or object spread. Mind to elaborate?

Give me a valid example of why the lack of implicit property initialisation in a class instance is an issue.

@RyanCavanaugh
Copy link
Member

@IgorSzymanski this continual moving of the goalposts isn't indicative of a good-faith conversation. If you don't think it's important to you, that's fine, but no one is required to justify spec compliance as a valid goal

@ajakaja
Copy link

ajakaja commented Jun 10, 2020

The reason I initially came across this issue, and wrote my comment above, is that I hit a case where an error was caused by this surprising logic. Unfortunately I cannot remember the exact example, but any similar example would be equivalent: if one writes

class Foo {
  public x?: string;
}

and doesn't initialize the property, there should be some sort of warning.

Otherwise

const foo = Foo()
console.log(foo.x); // typechecks, gives 'undefined'
console.log(foo.hasOwnProperty('x')); // gives 'false'

leads to confusion. For instance, sometimes one uses hasOwnProperty to implement a typeguard:

function isFoo(foo: Foo | Bar): foo is Foo {
  return foo.hasOwnProperty('x')
}

which fails if Foo is defined as above, even though it might be an actual instance of Foo.

@IgnusG
Copy link

IgnusG commented Mar 9, 2021

Any update on the spec compliance?

@RyanCavanaugh
Copy link
Member

This is now the behavior under --useDefineForClassFields

@ljharb
Copy link
Contributor

ljharb commented Mar 9, 2021

Is that also now the default in tsc init?

@RyanCavanaugh
Copy link
Member

Not yet, but we should probably change that.

@ljharb
Copy link
Contributor

ljharb commented Mar 9, 2021

Should this be reopened to track that, or would you prefer a separate issue?

@RyanCavanaugh
Copy link
Member

Separate issue please. Thanks!

@ljharb
Copy link
Contributor

ljharb commented Mar 9, 2021

Filed #43167.

@ExE-Boss
Copy link
Contributor

Looks like there’s already #39311.

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 ES Next New featurers for ECMAScript (a.k.a. ESNext) Suggestion An idea for TypeScript Waiting for TC39 Unactionable until TC39 reaches some conclusion
Projects
None yet
Development

No branches or pull requests