tc39 / proposal-class-public-fields

Stage 2 proposal for public class fields in ECMAScript
https://tc39.github.io/proposal-class-public-fields/
487 stars 25 forks source link

[[Set]] vs [[DefineOwnProperty]] #42

Open bakkot opened 8 years ago

bakkot commented 8 years ago

There's some debate over what the semantics should be. I wanted to try to collect arguments in one place.

Set Define
= implies Set. We could use a different sigil (although not :).
People expect to be able to copy code from the constructor to the class body and have it work the same. We don't currently have a syntax-level Define for classes, and we can or should use new syntax to do new things.
Most JS programmers do not distinguish between imperative and declarative position. It is a declarative position. In most cases Set and Define will behave identically, and programmers will have to learn which is which in the remaining cases anyway.
If a class defines a setter for foo to maintain some invariant, some people might be surprised that adding an initializer for foo shadows it instead of triggering it. Also, library authors use setters to deprecate properties (by allowing their use but printing a warning), and subclassers will only get those warnings if initializers use Set. In an object literal, { set foo(x){}, foo: 0 } does not trigger the setter.
Define will allow us to later introduce const properties or types. (And potentially decorators.)
Babel uses Set.

We also have the option of some hybrid strategy: for example, throwing if an initializer would overwrite another property and/or throwing if an initializer would shadow a property from the prototype.

bakkot commented 8 years ago

Ping @ljharb; do you see any I missed?

ljharb commented 8 years ago

When talking about a base class, your "object literal" example holds. However, when using extends, there's not really an analog to object literals (using __proto__ inline isn't really the same thing at all), so I think it's worth noting that the hazard mentioned on that row doesn't actually come up with object literals.

bakkot commented 8 years ago

Here's another, slightly more complex argument in favor of Define:

Because static methods and static fields are installed as properties of the same object, I would expect them to work very similarly. It would be very surprising if, for example,

class A extends B {
  static set foo(){}
  static foo = function(){};
}

triggered the setter, given that

class A extends B {
  static set foo(){}
  static foo(){}
}

redefines foo.

So (if you buy this argument) static fields ought to be created with DefineOwnProperty, presumably as writable, configurable, and non-enumerable. For consistency, non-static properties should be created the same way.

ljharb commented 8 years ago

Interestingly, Babel here doesn't actually redefine it, although Chrome native does.

bakkot commented 8 years ago

Babel gets class properties wrong in a number of ways - for example.

The spec is unambiguous (see step 21.b.i and here); Babel's just wrong.

domenic commented 8 years ago

I don't find that argument compelling. Consider the following code, which I claim is completely analogous:

var o = {
  set foo() {}
};
o.foo = function () { };

vs.

var o = {
  set foo() {}
  foo() { }
};

It is very unsurprising to me that the first triggers the setter and the second redefines.

bakkot commented 8 years ago

@domenic, I would think the closer analogy would be

var o = {
  set foo(x){},
  foo: function(){}
}

given that the field is inline, not after-the-fact.

domenic commented 8 years ago

Nah, that uses :, not =; you need = to keep the analogy.

bakkot commented 8 years ago

Eh, part of the discussion is over which sigil to use. Without considering the sigil, it seems that static field declarations ought to work the same as static method declarations.

bakkot commented 8 years ago

(Although even with =,

class A {
  static set foo(x){}
  static foo = function(){}
}

is more visually similar to

var o = {
  set foo(x){},
  foo : function(){}
};

than

var o = {
  set foo(x){}
};
o.foo = function(){}

to my eye.)

RyanCavanaugh commented 6 years ago

A subsequent duplicate declaration without an initializer isn't something we have to cook up never-written object literals to think about. JS already has this:

var x = 0;
var x;
console.log(x); // Does not print 'undefined'

The cognitive burden here just seems enormous for no actual upside. How would you explain this feature to a user?

Set

p = e; in the body is just sugar for writing this.p = e; at the top of the constructor

Accurate & is analogous to a thing people actually do today.

Define, v1

p = e; in the body is just sugar for writing Object.defineProperty(this, "p", { value: e }); at the top of the constructor

Sugar for a thing I never do? What is defineProperty exactly? Huh? What's the difference between that and this.p = e; ?

Define, v2

p = e; in the body is just sugar for writing this.p = e; at the top of the constructor unless there's a base class property with a setter in case your class will break for impossible-to-debug reasons that will ultimately lead to your team banning class property initializers because they block ergonomic updates to base class implementations

Is analogous to a thing people actually do today & spells out the consequences of using this feature when you're not in a position to inspect the implementation of the base class.

bakkot commented 6 years ago

A subsequent duplicate declaration without an initializer isn't something we have to cook up never-written object literals to think about.

To be clear, the double declaration was just to make the examples shorter; I should have written them out more. The code you'd actually see today would be more like

class Base {
  static set foo(x) {}
}
class Derived extends Base {
  static foo() {} // does not trigger setter
}

or

class Base {
  set foo(x) {}
}
class Derived extends base {
  foo() {} // does not trigger setter
}

or

let proto = {
  set foo(x) {}
};
let object = {
  __proto__: proto,
  foo: 0, // does not trigger setter
};

How would you explain this feature to a user?

Personally? "It works like an object literal." The point of classes is to be declarative, after all.

RyanCavanaugh commented 6 years ago
let object = {
  __proto__: proto,
  foo: 0, // does not trigger setter
};

I don't understand the value of analogizing to code that no sane person writes.

Re your other example, consider

class Base {
  set foo(x) {}
}
class Derived extends base {
  foo() {} // does not trigger setter
}

vs

class Base {
  set foo(x) {}
}
class Derived extends base {
  foo = () => {} // Different syntax that also doesn't trigger setter?
}

What happened to "new syntax should do new things" here?

"It works like an object literal."

Object literals don't have inheritance, so we're back to where we started in terms of describing behavior in terms of existing primitive operations. The closest thing is Object.assign but ES6 class inheritance already doesn't work anything like that.

bakkot commented 6 years ago

What happened to "different syntax should do different things" here?

Those do meaningfully different things already, as observable in the widespread if debatably advisable practice of using arrows in public fields to get "auto-bound" methods.

Object literals don't have inheritance

... Yes they do? Even without __proto__, they inherit from Object.prototype. I don't understand what you mean by this.

RyanCavanaugh commented 6 years ago

... Yes they do?

You can't write const obj = { x: 3 } extends { set x(v) { } }. There's no analogous syntax to what's happening with derived class properties regardless of what happens here.

michaelficarra commented 6 years ago

@RyanCavanaugh

const obj = {
  __proto__: { set x(v) { console.log('not called'); } },
  x: 3,
}

or

Object.defineProperty(Object.prototype, 'x', {
  set: v => { console.log('not called'); },
});
const obj = {
  x: 3,
}
ljharb commented 6 years ago

Or Object.create

RyanCavanaugh commented 6 years ago

I repeat my earlier claim of the uselessness of analogizing to code that nobody writes.

Neither behavior is unambiguously "correct"; there are multiple plausibly-analogous behaviors we can point to (constructor assignment, prototype chaining, object literal property assignments, simple assignment, etc).

The question is which behavior is going to be dangerous or unexpected. Are people writing { __proto__: someObj, x: e } in regular code with the clear expectation of squashing setters in someObj? My experience is not; I would be interested to see evidence otherwise.

Are people writing this.foo = bar in their constructor bodies, possibly triggering base class setters in the process? We absolutely know that they are.

Are people writing Object.defineProperty(this, "foo" in their constructor bodies with the clear intent of overwriting base class setters? I have not seen this code and I would wager you can't find any.

Should the behavior of an initializer be like the common thing that people do all the time, or should it be like a dangerous other uncommon thing that is going to subtly break base class behavioral expectations?

jeffmo commented 6 years ago

When the issue of define vs set first came up we explored it quite a bit. The conclusion we ultimately came to was that neither is 100% obviously better than the other (both have decent arguments for and against them), but also those pros/cons aren't exactly fatal.

Set

The predominant argument in favor of [[Set]] is that it satisfies use cases similar to the following:

class MyTextNode extends TextNode {
  textValue = "howdy!";
}

It's reasonable for a user to expect that this would create a DOM text node with the text howdy! inside -- but in fact it wouldn't with define, because TextNode works via setters. The fix for this with define would be to move the assignment into the constructor:

class MyTextNode extends TextNode {
  constructor(...superArgs) {
    super(...superArgs);
    this.textValue = "howdy!";
  }
}

This is roughly equivalent to what users must do today in this situation, and one could even argue that since the subclass doesn't "own" the field it probably shouldn't be declaring it either. (this is debatable and I don't care to debate it again -- I'm just pointing out that it is indeed debatable)

Define

The main benefits of using Define were that it leaves room for the addition of const fields (and Mark's const classes) in the future, and it plays a little more seamlessly with decorator-descriptors. If we were to go with Set and one day were to add const fields, it would lead to some pretty surprising/problematic refactoring cases if one updated foo = 42; --> const foo = 42;.

So at the end of the day, you really can make reasonable and non-fatal arguments in favor of and against both of these things. So the fact that going with Set could easily make it more difficult to add const fields in the future and somewhat simplifies the interactions between field initialization and decorators, we chose to go with define.

hax commented 6 years ago

addition of const fields (and Mark's const classes) in the future

Where are these proposals? Links?

hax commented 6 years ago

So the fact that going with Set could easily make it more difficult to add const fields in the future

If you want const foo = 42 in the future, why the syntax is not let foo = 42 now??

jeffmo commented 6 years ago

If you want const foo = 42 in the future, why the syntax is not let foo = 42 now?

The reason let was dismissed as a prefixing keyword is because it already means “lexical variable declaration”, which is not appropriately similar how class fields behave.

I should also note that const foo = ... would suffer from the same problem, but we haven’t (hadn’t?) explored const fields enough to suggest any kind of definitive syntax for them. The issue is about preserving room for consistent semantics, which generally comes before deciding on a syntax.

hax commented 6 years ago

@jeffmo While if you dismissed let, you should also dismiss const. Maybe you can use final...

jeffmo commented 6 years ago

Yes I agree, I think a future const fields proposal should probably dismiss the const foo = syntax in favor of some other syntax.

Let’s try to keep this thread on topic though :)