Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

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

Open
dfahlander opened this issue Oct 1, 2020 · 14 comments

Comments

@dfahlander
Copy link

dfahlander commented Oct 1, 2020

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
Copy link
Member

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
Copy link

trusktr commented Oct 6, 2020

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
Copy link

rdking commented Oct 8, 2020

@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
Copy link

trusktr commented Nov 26, 2020

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

@rdking
Copy link

rdking commented Dec 1, 2020

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
Copy link
Member

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

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

@rdking
Copy link

rdking commented Dec 8, 2020

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
Copy link
Author

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
Copy link
Member

Would name; effectively be a comment in that case?

@ljharb
Copy link
Member

ljharb commented Dec 8, 2020

@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
Copy link
Author

dfahlander commented Dec 8, 2020

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
Copy link
Author

dfahlander commented Dec 8, 2020

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
Copy link

trusktr commented Jan 24, 2021

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

🐄 💩

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

@trusktr
Copy link

trusktr commented Jan 24, 2021

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

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

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

No branches or pull requests

5 participants