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

[[Define]] semantics necessitate workarounds #322

Open
trusktr opened this issue Sep 30, 2020 · 7 comments
Open

[[Define]] semantics necessitate workarounds #322

trusktr opened this issue Sep 30, 2020 · 7 comments

Comments

@trusktr
Copy link

trusktr commented Sep 30, 2020

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

ljharb commented Sep 30, 2020

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

trusktr commented Sep 30, 2020

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

ljharb commented Sep 30, 2020

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

trusktr commented Sep 30, 2020

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

trusktr commented Oct 6, 2020

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

trusktr commented Oct 6, 2020

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

trusktr commented Oct 6, 2020

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.

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

2 participants