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

[[Define]] semantics necessitate workarounds #322

Open trusktr opened 4 years ago

trusktr commented 4 years ago

I had code like the following, which worked great with [[Set]] semantics (f.e. in TypeScript with useDefineForClassFields set to false):

class Sub extends Node {...}
const s = new Sub({prop1: 1, prop2: 2})
class Sub2 extends Sub {...}
const s2 = new Sub2({prop1: 1, prop2: 2, prop3: 3})

but due to [[Define]] semantics, I had to change it to

class Sub extends Node {...}
const s = new Sub().set({prop1: 1, prop2: 2})
class Sub2 extends Sub {...}
const s2 = new Sub2().set({prop1: 1, prop2: 2, prop3: 3})

because of [[Define]]'s accessor-overriding nature.

I like the new set() API better (because it duals as an API for setting properties at any time and via constructor, not just via constructor), but the main point is I was forced to make this change, whereas otherwise I could've opted into making this API later (and it would work regardless of [[Set]] vs [[Define]]) and avoided making a breaking change.

ljharb commented 4 years ago

I'm not sure why this change was forced; you could have used this.a = prop1 in the constructor instead of a class field and it'd have used Set semantics.

trusktr commented 4 years ago

Changing from foo = 123 to this.foo = 123 in pre-existing code is a forced change.

The pre-existing code also uses decorators, and ergonomics would be lost in moving things into the constructor. For big classes, this would mean even more refactoring (where private properties are co-located with methods).

ljharb commented 4 years ago

I agree it's a forced change - but it's a forced change within your class that does not cause a breaking change, and it's only caused by using pre-stage 3 TC39 proposals.

trusktr commented 4 years ago

it's a forced change within your class that does not cause a breaking change

Sure, I can change how my class is implemented, so that people instantiating my class get the same API without a breaking change.

But there's consumer code that creates subclasses following the same patterns, and they need to make a change too.

For example, suppose the above Sub class is in a library A, and Sub2 is consumer code in library B that wishes to provide the same usage patterns for a hypothetical Sub3 class in yet another library or app. Now the author of library B experiences a breaking change in library A which has modified the method by which classes need to be defined. And then that propagates to library C, especially if those authors would like to still support both [[Set]] and [[Define]] semantics (for new code, and for old code still running in Babel loose more or TypeScript without useDefineForClassFields).

trusktr commented 4 years ago

Updated my previous comment to make it more clear. There does not need to be a breaking in how people use instances of those classes, but there does need to be breaking changes in how consumers define new subclasses.

trusktr commented 4 years ago

Another thing is that to fix various libraries that previously always worked fine (with [[Set]] semantics), and we wish to avoiding re-writing the implementation, we have to do annoying things like this:

Before:

export class ConsumerClass extends LibraryClass {
  // ... before class fields, there was only [[Set]] in here, unless someone use `Object.defineProperty` but that's considered a clear indicator of overriding base behavior class.
}

Now:

export const ConsumerClass = LibraryMixin(class ConsumerClass {
  // ... before class fields, there was only [[Set]] in here, unless someone use `Object.defineProperty` but that's considered a clear indicator of overriding base behavior class.
})

And the reason for such a change is so that the library code can run after fields are [[Define]]ed.

This whole thing is such a mess, and who knows when decorators are coming out, and specs should not break ecosystems for very insignificant gains.

trusktr commented 4 years ago

Most comments that I post in this GitHub issue tracker happen indirectly because I encountered an issue in real-world code and then came here to post a comment. It's just true that [[Define]] has caused various headaches.

I can avoid ever using class fields, but I can't force consumers of all my code to do that, so things still won't work one way even if I want it to.