tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Properties without initializers can create some headache in "real-world" scenarios. #323

Open dfahlander opened 4 years ago

dfahlander commented 4 years ago

I want to share something that can become a little of a headache for my users of the Dexie.js library. I'm not advocating for a change (because I haven't deep-dived into the implications), just want to share this so there is an awareness of this issue.

I'm the author of Dexie.js and have had to deal with this lately - to overcome the problems that occur due to the fact that uninitialized properties are implicitly initialized to undefined under the hood. And that does not play well with IndexedDB in certain scenarios.

Basically, we have a DOM system (IndexedDB) that distinguishes between {name: "Foo"} and {id: undefined, name: "Foo"}. The latter will fail in the call to IDBObjectStore.prototype.add() if the object store has the autoIncrement flag. This becomes a real problem for current Typescript users as the declaration of fields are for the purpose of typing - not initializing. I have a feeling this could be an issue generally on various libs - not just Dexie or IndexedDB libraries that change behaviour due to existing properties. And the user's intent of the prop declaration is not very obvious.

Example

class Friend {
  id?: number;
  name: string;

  constructor(name) {
    this.name = name;
  }
}

const db = new Dexie("friendsDB");
db.version(1).stores({
  friends: "++id, name" // "++id" makes the key autoIncremented
});

await db.friends.add(new Friend("foo"));

With babel's "plugin-transform-typescript" the class Friend transpiles to

class Friend {
  id; // Here's the problem. id will be set to undefined.
  name;

  constructor(name) {
    this.name = name;
  }
}

I've "fixed" this problem in Dexie by working around the specific scenario by rewriting the input object and removing undefined props from it before passing it to IDB. But what other systems out there will have to introduce workarounds due to this behavior?

nicolo-ribaudo commented 4 years ago

If you don't want the property to be set to undefined, TypeScript allows you to mark it as a type-only annotation:

class Friend {
  declare id?: number;
  name: string;

  constructor(name) {
    this.name = name;
  }
}
trusktr commented 4 years ago

That's a workaround only for TypeScript code (good thing for your case). No way around it in plain JS. Try in console:

class Foo { foo }
const f = new Foo
console.log(f.hasOwnProperty('foo'), f.foo) // true, undefined

I mean, no way in plain JS without moving into the constructor instead.

what other systems out there will have to introduce workarounds due to this behavior?

This causes headaches for quite a bit of people.

rdking commented 4 years ago

@trusktr Actually, there is a way around it. Put the default data on the prototype.

function makeProto(obj) {
   let retval = function () {};
   Object.assign(retval.prototype, obj);
   return retval;
}

class Friend extends makeProto({
   id: undefined
}) {
   name: undefined;
   constructor(name) {
      this.name = name;
   }
}

Done this way, id is neither being moved to the constructor, nor an own property of the instance until a value is set on it. It's amazing how often the prototype-based nature of this language is neglected.

trusktr commented 3 years ago

That works, but it isn't desirable to write code that way.

rdking commented 3 years ago

I agree that it's by no means desirable to have to resort to such constructs. However, it's also not like there's much in the way of better options. Defining the field on the instance at construction blows the point of having fields. Not declaring the field is a no-go for Typescript users. Are there any other possibilities?

nicolo-ribaudo commented 3 years ago

Not declaring the field is a no-go for Typescript users.

This is a solved problem, since TS uses the declare keyword.

rdking commented 3 years ago

This is a solved problem, since TS uses the declare keyword.

My comments were based on the need to "declare" but not reify a class member on the instance in ES without Typescript, which doesn't have a clean, easy solution to my knowledge. The only way I know to accomplish this is to use the prototype since that's exactly what it's for. However, I know that's unpopular. So when I asked

Are there any other possibilities?

I was (and still am) looking to see if there are other viable options. When I said

Not declaring the field is a no-go for Typescript users

I was referring to the fact that the lack of a declaration for a field in an ES module can pose headaches for TS users trying to make use of the module.

dfahlander commented 3 years ago

Maybe an option would be to ignore class fields without an initializer and treat them as a no-op as they are generally a bad idea in plain JS and lead to double set calls otherwise.

class Friend {
  name;

  constructor(name) {
    this.name = name;
  }
}

I don't see the benefit with first setting name to undefined and then setting it in the constructor.

Alternately to treat non-initialized class field properties as a syntax error.

nicolo-ribaudo commented 3 years ago

Would name; effectively be a comment in that case?

ljharb commented 3 years ago

@dfahlander in that case you wouldn’t likely want the class field at all, unless you were trying to shadow a superclass setter.

Public fields aren’t something you generally do or need to “declare”, absent a type system.

dfahlander commented 3 years ago

I still find class fields very nice for initializing fields without having to declare a contructor. Also to block the property from a get/set property in the parent class.

class FriendList {
  friends = new Map();

  add(name, number) {
    this.items.add(name, number);
  }

  ...
}

I'm only afraid that people might start using class field for declarative purposes without knowing that they implicitly initialize the property not knowing this will break code. Not sure this applies to typescript users only but of course it would hit typescript users the most. In case the ES spec only allowed explicit initialization on class fields, the Typescript team would have to transpile their class fields differently, which would be a good thing, as it would break less code.

In case class fields would be used to hide a setter of the super class, a more explicit class declaration would be fine for me:

class Child extends Parent {
  name = null; // Explicitely initialize it

  constructor(name) {
    this.name = name;
  }
}

class Parent {
  get name() { return localStorage.get('name'); }
  set name(value) { localStorage.set('name', value); }
}

As we already know, the class field name on Child prevents it from being stored in localStorage as the Parent class defines the property. Just illustrating how the code would have to be written if explicit declaration was required, which is the thought I am proposing here.

dfahlander commented 3 years ago

Would name; effectively be a comment in that case?

Yes, or treat it as a syntax error, just like name: string; is a syntax error (but still perfectly allowed in typescript / flow).

trusktr commented 3 years ago

I'm only afraid that people might start using class field for declarative purposes without knowing that they implicitly initialize the property not knowing this will break code

Exactly!

I'm sorry to say, but TC39

ruined

ES class properties.

I was shot in the foot by this yet again today.

I've wasted colossal amounts of time due to this issue.

It's a big mistake.

And now people entering into the JavaScript world have to deal with the

:cow2: :hankey:

I feel sorry for them, and sorry for what people will think of JavaScript.

trusktr commented 3 years ago

I've published code to production, and the issues that stem from this didn't become apparent until later. :exclamation: :exclamation: :exclamation:

Testing would have caught the problem, but you all know 100% code coverage is often never achieved.