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

Can we take a step back? #144

Closed rdking closed 5 years ago

rdking commented 5 years ago

When there is so much feedback on this proposal from those who are aware and choose to speak at all and from members of the TC39 board itself that there are several things wrong with this proposal that make it a less than adequate solution to the problem of adding data privacy to ES, isn't that a good enough indication that it may be time to step back and re-think?

How about, instead of trying to push through with class-fields, we come up with "class properties" instead? The general idea is simple. Objects have properties. Functions are objects. Classes are functions with a prototype object. Therefore classes should support declaring properties for both its function and its prototype.

Can we solve and add this to the language first, separately from all other concerns? No private, no protected or other intermediate. Just ordinary public and static properties. I'm beginning to come to the impression that until this much is solved, the path forward for a reasonably acceptable, non-future path blocking proposal won't easily reveal itself. Maybe we're trying to take too big of a jump all at once.

rbuckton commented 5 years ago

You can accomplish this with fields and decorators:

// class fields + decorators
function proto(memberDesc) {
  if (memberDesc.kind !== "field" ||
      memberDesc.placement !== "instance") throw new Error("Only valid on non-static fields");
  memberDesc.placement = "prototype";
}

class C {
  @proto propertyOnPrototype = 1;
}

The biggest issue with declaring data properties other than methods on the prototype is related to shared instances of non-primitive values:

class C {
  state = { counter: 0 }; // on instance
  incr() { console.log(this.state.counter++); }
}

class D {
  @proto state = { counter: 0 }; // on prototype
  incr() { console.log(this.state.counter++); }
}
const c1 = new C();
c1.incr(); // prints: 0
c1.incr(); // prints: 1

const c2 = new C();
c2.incr(); // prints: 0
c2.incr(); // prints: 1

const d1 = new D();
d1.incr(); // prints: 0
d1.incr(); // prints: 1

const d2 = new D();
d2.incr(); // prints: 2
d2.incr(); // prints: 3

In most OOP languages, state shared between instances is placed on the "static" side of the class.

rdking commented 5 years ago

@rbuckton I think you may have missed the point of what I'm saying. Right now class is equivalent to a constructor function and a prototype with all defined prototype elements defined as non-configurable, non-enumerable, writable. If doing this manually, data properties can be placed on the prototype with default values. The problem of assigning a property an object for a default value has always existed. This makes it a non-issue in my book. However, in a class, there is no direct means of making the same assignment.

I'm saying that this:

class Foo {
  x = 1;
  inc() { ++this.x; }
}

being an equivalent for:

const Foo = (function() {
  var retval = function Foo() {};
  retval.prototype = {
    constructor: retval,
    x: 1,
    inc() { ++this.x; }
  };
  return retval;
})();

needs to be implemented before we go any further with any kind of private data proposal. Let's get public working first.

jridgewell commented 5 years ago

with all defined prototype elements defined as non-configurable, non-enumerable, writable

Nit: They're configurable, writable, and non-enumerable.

The problem of assigning a property an object for a default value has always existed. This makes it a non-issue in my book. However, in a class, there is no direct means of making the same assignment.

This is an anti-pattern, and should be avoided at all costs. It's been the source of countless bugs in Backbone (which has a very similar class system).

That class fields syntax does not allow you to do this natively is a great positive. If you want to shoot yourself in the foot later, mutate the prototype after the class' curly brace.

mbrowne commented 5 years ago

I think having a way to declare instance properties is much more important than a dedicated syntax to declare prototype properties. The only way this could work without causing major usability issues would be for it to detect non-primitive (object) initializers and set those to null on the prototype and set the initializer value on the instance in those cases...which sets up an inconsistency. I’m not saying it’s totally unworkable but the current proposal is much simpler (at least if we’re just comparing public properties) and between static and decorators we’ll have all the power we need to affect placement.

This idea would indeed be taking a step back, but not for the better...

The following comments might be a better fit for the other thread but...The problem with the “maximally minimal” approach is:

  1. JS has been suffering from the lack of proper encapsulation for so many years already and it seemed that in past proposals something else was always a higher priority. It’s time to finally make it happen. If private fields were removed from this proposal, how much longer would we have to wait? If it would be 6 months after the ratification of this proposal, perhaps that would be acceptable but isn’t it hard to know how long it might take? Perhaps years, especially given how controversial this clearly is...

  2. Public and private fields/properties are closely related and it makes sense to consider both and release them together. I would say this extends to access control on general if it weren’t for number 1 above. But this is why I keep insisting on a general plan for how other access levels might be handled, because the decisions made now - in some cases even decisions just related to public properties/fields - will very significantly affect any future proposals. BTW I think it would be great if this proposal and decorators were released very close together, because hard private on its own with no way to modify or expose it for legitimate use cases could cause lots of problems.

@hax

rdking commented 5 years ago

There's a much more straight forward solution than not using the prototype at all. In much the same way as ** doesn't allow non-constant expressions as the LParam due to the multiple ways get the wrong result, simply disallow assignment of an object to a class property definition unless the property is static. Cleanly solves the problem, that is, until you have to deal with variables. Then things get messy. That's why the real suggestion is translating this:

class Foo {
  x = {foo: 'bar'};
  print() { console.log(JSON.stringify(this.x, null, '\t')); }
}

into something like this:

const Foo = (function() {
  const isSet = Symbol()
  var retval = function Foo() {};
  retval.prototype = {
    constructor: retval,
    get x() {
      if (!(this && (typeof(this.x) == "object") &&
          this.x[isSet])) {
        this.x = { foo: 'bar' };
      }
      return this.x.value;
    },
    set x(value) {
      this.x = {
        [isSet]: true,
        value
      };
    },
    print() { console.log(JSON.stringify(this.x, null, '\t')); }
  };
  return retval;
})();

The general idea here is to defer the setting of properties just like is being done for the current proposal. The only difference is that the property appropriately lives on the prototype, as is expected.

mbrowne commented 5 years ago

@rdking

The only difference is that the property appropriately lives on the prototype, as is expected.

As is expected by whom? Even before JS had classes I was already declaring data properties in the constructor and methods on the prototype. It would only be in special cases where I added data properties to the prototype. And while of course some people did things differently, this seemed to reflect most of the examples and common practice. And now many people have already been using public fields via Babel and having no problems with them, and the default placement of an instance property seems to work well for people.

rdking commented 5 years ago

@mbrowne As you should be able to see from my previous post, there's a solution that gives you all the benefits of instance "fields" without going all the way to the "field" abstraction. I can't really see any reason why we need to go as far as something like "fields" to do what ES can already do with properties.

rdking commented 5 years ago

@mbrowne There are going to be those who do things as you do, and those who do things as I do. The two groups will likely never be reconciled. However, the point is to take an approach that allows for both methods to be used. Embedding an approach that only considers one way or the other does a disservice to those using the opposite approach. What I've suggested above gives both.

hax commented 5 years ago

@jridgewell

If you want to shoot yourself in the foot later, mutate the prototype after the class' curly brace.

But current proposal just shoot yourself in another direction.

Code sample which I already pasted several times and no one respond:

class ExistingGoodClass {
  get x() {...}
  set x(v) {...}
}
class NowTrapAgain extends ExistingGoodClass {
  x = 1
}

Note, this footgun have different variant forms if you considered how code evolve.

rdking commented 5 years ago

@hax @jridgewell The solution I'm suggesting avoids the gun altogether. That example would translate to

class ExistingGoodClass {
  get x() {...}
  set x(v) {...}
}
var NowTrapAgain = (function() {
  function getGetter(obj, field) {
    var retval;
    while ((typeof obj == "object") && !obj.hasOwnProperty(field)) {
      obj = Object.getPrototypeOf(obj);
    }
    if (obj.hasOwnProperty(field)) {
      retval = Object.getOwnPropertyDescriptor(obj, field).get;
    }
    return retval;
  }

  return class NowTrapAgain extends ExistingGoodClass {
    get x() {
      var data = 1; //This is the original assigned value
      var getter = getGetter(this, "x");
      if (this !== Test.prototype) {
        if (!getter.data) {
          getter.data = new WeakMap();
        }
        if (!getter.data.has(this)) {
          getter.data.set(this, data);
        }
      }
      return (this === Test.prototype) ? data : getter.data.get(this);
    }
    set x(val) {
      /* It can be solved in the engine possibly, but from source, there's no solution that
       * would allow perfect object re-instantiation given a data parameter, so changing
       * the prototype value is out.
       */
      if (this !== Test.prototype) {
        var getter = getGetter(this, "x");
        if (!getter.data) {
          getter.data = new WeakMap();
        }
        getter.data.set(this, val);
      }
    }
  }                 
})()

The basic principle here is to split the property into a getter & setter, and allow them to manage the instantiation of the data. Done this way, neither of the foot guns you two mentioned exist.

loganfsmyth commented 5 years ago

@hax To make sure we're on the same page, that snippet will not trigger the setter, it will define a new property with the value 1. Which behavior do you consider a footgun?

hax commented 5 years ago

JS has been suffering from the lack of proper encapsulation for so many years already and it seemed that in past proposals something else was always a higher priority. It’s time to finally make it happen.

Technically speaking, I agree "proper encapsulation" is the highest priority in all scope of this proposal covered. And I sincerely hope it could happen.

If private fields were removed from this proposal, how much longer would we have to wait? If it would be 6 months after the ratification of this proposal, perhaps that would be acceptable but isn’t it hard to know how long it might take? Perhaps years, especially given how controversial this clearly is...

As a man who already have writing JavaScript for 20 years, and plan to keep writing for another 20 years, I don't care waiting another 2 years, because if you landed a broken solution, I would suffer 20 years.

To be honest, the semantics of the private part of this proposal is ok to me. I even considered the # syntax was acceptable.

But as I already said, after I gave a speech, and got the feedback, I just realized, we should never underrate the risk of the community break. And even I see some other controversial issues in other proposals, the risk of no one can compare to this.

Why?

Because there are already too many solution for private in the wild! If you just land a unwelcome proposal like this, you are just add another mess even you are literally "standard".

This is especially true in TS land, because TS already have an official, solid, programmers familiar private solution. You can see my analysis in other thread.

  1. Public and private fields/properties are closely related and it makes sense to consider both and release them together.

No. They are not. Public is not essential as private. Actually public field has been proved as a bad idea in other OO languages like Java, C#. Though the reason why they discouraged it not necessarily applied to JS, if you checked deeply, you just found more issues in JS than in Java/C#.

Note, the historical property usages in JavaScript is not equal to "public field declaration" you introduced in this proposal. There is a subtle but significant difference, that you provide a footgun that subclass could easily (and accidently in almost all the cases) redefine a property which is already defined by base class. This footgun is never available in the past unless someone explicitly write Object.defineProperty in the constructor --- no one write code like that, and if there is anyone, then that means he should definitely know what he was doing and no regret. I have no idea why we have to introduce such footgun.

hax commented 5 years ago

Which behavior do you consider a footgun?

Yes you know it define a prop, but average programmers just think he do the same as constructor() { this.x = 1 }

slikts commented 5 years ago

… TS already have an official, solid, programmers familiar private solution.

The privacy in TS does close to nothing, because TS users can just opt-out of types to use private APIs, and it doesn't affect anyone else. Moreover, TS was never meant as a competing standard, so the image is not relevant, and it's getting a bit too spammy.

hax commented 5 years ago

There are some options to solve this:

  1. Add public or any other keyword to make it explicit here is a declaration. Though it's still footgun, at least programmers can see it! But unfortunately you will not like the consequence of add keyword which ask private #x and other syntax storm.
  2. Do not allow initialization, which never confused with assignment. Though it's still footgun, programmers has no reason to use it, so they are probably safe. But I think you will not accept it...
  3. Use constructor() { this.x = 1 } semantic, then it's not a public field proposal 🤣 , it just allow you to move the assignment out of constructor.

I think you don't want to any of them. So just let the footgun shoot ourselves... 😭

hax commented 5 years ago

@slikts

The privacy in TS does close to nothing, because TS users can just opt-out of types to use private APIs, and it doesn't affect anyone else. Moreover, TS was never meant as a competing standard, so the image is not relevant, and it's getting a bit too spammy.

Ok, you can keep your judgment which have no any proof.

Or you should ask TS guys how they think about it.

slikts commented 5 years ago

Sorry, proof about what? Here's TS playground demonstrating privacy being stripped away in JS and being optional in TS. Here's TS design goals, which include tracking JS.

rbuckton commented 5 years ago

@rdking: Field initializers cannot be lazy by default as you propose in https://github.com/tc39/proposal-class-fields/issues/144#issuecomment-429914655 as it would cause strange side effects and unexpected behavior:

let idCounter = 0;
class C {
  id = idCounter++;
}
const a = new C();
const b = new C();
b.id; // 0, but expected 1
a.id; // 1, but expected 0

The semantics of field initializers mimic the behavior of field initializers in Java/C#, in that initializers are processed in the constructor after superclass constructors have been evaluated.

Privacy in TS has been well known to be a "soft" private, relying on design time behavior to inform the user that the API is not safe to depend upon. It is not and was not designed as a security feature, but more of a way to indicate to users parts of an application that you should not take a dependency on. It is akin to /** @private */ in JSDoc, only you can rely on it more as it becomes an error during build.

rbuckton commented 5 years ago

@rdking: Field initializers cannot be lazy by default […]

Note that you could, however, create a decorator that performs lazy initialization. However, unlike the current TS behavior (which uses Set and would trigger setters on the prototype), the class fields proposal uses CreateDataPropertyOrThrow (which won't trigger setters on the prototype).

hax commented 5 years ago

Sorry, proof about what? Here's TS playground demonstrating privacy being stripped away in JS and being optional in TS. Here's TS design goals, which include tracking JS.

Ok if you are talking about hard private, I never against it. I just want to say, the difference between hard private and TS compile-time private doesn't very important in practice, at least in most cases.

And, I think TS eventually should move to one private, it should be JS native private, as you say, the design goal of TS is to follow JS semantic.

Because of that, you should realize the good proposal should not only consider JS, but also consider TS.

If a proposal can not satisfy TS programmers or bring them pain, they will keep using TS compile-time private.

Could you get my point?

hax commented 5 years ago

@rbuckton

The semantics of field initializers mimic the behavior of field initializers in Java/C#, in that initializers are processed in the constructor after superclass constructors have been evaluated.

I'm afraid C# has different order with Java. I prefer C# but it seems current proposal choose Java?

rbuckton commented 5 years ago

@hax: How do they differ? While I admit I've spent considerably more time in C# than Java, the initialization semantics in https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.5 seem fairly similar to C#'s semantics.

mbrowne commented 5 years ago
class ExistingGoodClass {
  get x() {...}
  set x(v) {...}
}
class NowTrapAgain extends ExistingGoodClass {
  x = 1

IMO it would be better if redeclaring the same property on a subclass in this way were illegal. It’s a recipe for confusion and problems. Overriding the getter and setter would probably be OK but overriding it with a public field declaration is an issue. I’m also against this:

class Superclass {
  x = 1
}
class Subclass extends Superclass {
  x
}
rdking commented 5 years ago

@rbuckton Given your example, I'd expect a.id === b.id && a.id === 0 && idCounter === 1. My reason? I expect a definition to be descriptive, not prescriptive. Put another way:

let idCounter = 0;
let C = function() { //There's no constructor in C! }
C.prototype = {
  constructor: C,
  id: idCounter++;
};
const a = new C();
const b = new C();
b.id; // 0
a.id; // 0

Justification? Currently everything inside the class definition goes on the prototype of the class, including the constructor function. The class keyword is just sugar for building a prototype with a default constructor if one isn't provided, and then returning the constructor. Adding data property definitions to the class keyword shouldn't suddenly up and do something different. Besides, accessor property definitions end up on the prototype already, right?

rbuckton commented 5 years ago

@rdking: That is not how class fields behave in any other OOP language. If those initializers defined properties on the prototype, the feature would be a major foot gun and pretty much unusable. Prototypes should define instance shared behavior. Even with classic ES5/3 "classes", defining non method or accesor members of a shared prototype was a foot gun and generally to be avoided.

rdking commented 5 years ago

@mbrowne From a different point of view, I agree that

it would be better if redeclaring the same property on a subclass in this way were illegal.

But the problem is that the re-declaration is a "field" and not a property. If the assignment remained in the constructor, there would be no problem. However, assigning a value in a definition sets up the expectation that the value will be present before the first line of constructor code is run (that includes super()).

Put simply. If you want an instance field, then make it after the instance is created (i.e. in the constructor). However, if you want a class property, it needs to be part of the prototype. There's no issue with overriding a base class property with a derived class property. That's the very basis of prototype inheritance.

rdking commented 5 years ago

@rbuckton You might want to double-check your facts. Both C++ and Java work that way. Values assigned to properties of a class are done so statically so that an internal prototype of the exact shape of a class instance can be created. This is why sizeof works. The resulting assignments to the properties of the instances created are the values that were captured from the definition. If you wanted to assign a value that's dynamic to a field, you have to do it from the constructor.

bakkot commented 5 years ago

@rdking:

import java.util.*;
import java.lang.*;
import java.io.*;

class Main {
    static int idCounter = 0;
    static class C {
        public int id = idCounter++;
    }
    public static void main (String[] args) {
        C a = new C();
        C b = new C();
        System.out.println(b.id);
        System.out.println(a.id);
    }
}

output:

1
0

Try it.

sizeof does not need to know the actual value of anything, only its type.

Sidebar: I'd really appreciate it if you would create and run (and ideally provide) code samples before making claims about the behavior of existing languages.

rbuckton commented 5 years ago

This is a big difference between a typed language like C# and an untyped language like JavaScript. sizeof in these languages works because the metadata about the shape of the class (the declared members and types) exists as part of the class declaration. The actual values are not initialized until such time as the constructor runs.

The difference between ECMAScript and C# constructors, is that C# does not allow you to evaluate Statements prior to calling the base class constructor:

// c#
class C {
  public int x;
  public int y = 1;
  public C(int x) {
    this.x = x;
  }
}
class D : C {
  public D(int x): base(x) {
    // cannot execute statements before `base(x)` is evaluated.
    Console.WriteLine(this.x);
  }
}

vs

// js
class C {
  x;
  y = 1;
  constructor(x) {
    this.x = x;
  }
}
class D extends C {
  constructor(x) {
    // can execute statements here, but `this` is not initialized yet.
    super(x);
    console.log(this.x);
  }
}
rbuckton commented 5 years ago

If you really do want a default value on the prototype, you can still achieve this via decorators:

function defaultValue(value) {
  return function(fieldDescriptor) {
    fieldDescriptor.extras = [{
      placement: "prototype",
      kind: "field",
      key: fieldDescriptor.key,
      initializer: () => value
    }];
  };
}

let idCounter = 0;
class C {
  @defaultValue(0)
  id = idCounter++;
}

const a = new C();
const b = new C();
console.log(a.id); // 0 - from initializer
console.log(b.id); // 1 - from initializer
console.log(C.prototype.id); // 0 - from @defaultValue
rdking commented 5 years ago

@rbuckton Well heck. Now I'm going to have to retract my previous argument. After C++-11, they modified it so that the notation you're describing works. They probably did the same thing to Java somewhere around 1.3.... So you're right. I need to double check my info to make sure it's not outdated first.

rdking commented 5 years ago

@rbuckton Here's a thought. It's the assignment that's the issue here. I'd still posit that declared data elements in a class should be prototype properties. The problem is the initialization of those properties. For good reasons, the current proposal is putting those in the constructor, but that's not happening soon enough.

Ignoring my earlier ignorance of the language changes in that area, there's still a major difference that's important to consider. ES is a "prototype-based" language where Java, C++, C#, etc are "class-based". The rule that defines the type in those languages is the class, while in ES, it's an object, namely the prototype. If it's not part of the prototype, it's not part of the type. The instanceof keyword follows this exact same notion. The expectation of data elements in a class is that they are part of the type. Thinks haphazardly pinned on after the fact are not part of the type. Even the way the current proposal handles access to private members follows this notion.

Compound all of that with the fact that developers are already accustom to doing their data definitions in the constructor, and it becomes reasonable that either:

As it presently stands, the first of those 2 is already the current state of affairs. As we all are seeking a way to add support for private data, I think we need to seek the second option. Every other approach is full of pitfalls of one kind or another. Even the current proposal fails to allow for a subclass overriding the public data elements of a derived class. This gets even worse if some kind of intermediate access level is ever implemented.

bakkot commented 5 years ago

@rdking:

If it's not part of the prototype, it's not part of the type. [...] The expectation of data elements in a class is that they are part of the type.

I do not share your intuitions here. Nor, as far as I can tell, do the vast majority of JavaScript users, who are generally accustomed to performing an assignment to a property in a constructor and then treating all instances of the corresponding type as having that property.

Sidenote: I think this would be easier to talk about if you would say "my expectation" rather than "the expectation".

Even the current proposal fails to allow for a subclass overriding the public data elements of a derived class.

I don't know what this means. This

class Base {
  x = 0;
}
class Derived extends Base {
  x = 1;
}
console.log((new Base).x); // 0
console.log((new Derived).x); // 1

works fine.

rdking commented 5 years ago

@bakkot I never said it was the only, or everyone's, or even most developer's expectation, but I get what you mean. Where possible, I'll trim back.

Even the current proposal fails to allow for a subclass overriding the public data elements of a derived class.

I don't know what this means.

class Base {
  get x() { ... }
  set x(v) { ... }
}
class Derived extends Base {
  x = 1;
}

The current proposal doesn't properly support this.

bakkot commented 5 years ago

The current proposal doesn't properly support this.

It creates an own data property shadowing the accessors, as I think it should, since that's what it looks to me like it's declaring.

I know you think it should be something else. But the current behavior not wrong; it's just not what you want. (I understand the reasons to want it. I still disagree.)

Again, I think this would be a lot easier to talk about if you would say that the current behavior is not what you want, rather than that the current behavior is improper.

rdking commented 5 years ago

@bakkot According to what I've learned from you about the current proposal

class Derived extends Base {
  constructor() {
    super();
    this.x = 1;
  }
}

is the equivalent of the example above. Currently, this would cause the setter for x on Base to be called. Shadowing the inherited accessor pair is unexpected behavior in this example. That's what I was referring to as improper behavior.

bakkot commented 5 years ago

It is not equivalent - it differs in that the snippet you just posted uses [[Set]] semantics, rather than (as would happen if you used a field definition from this proposal) [[DefineOwnProperty]] semantics.

(In all other ways it's the same, though.)

rbuckton commented 5 years ago

@rdking: As @bakkot said, the equivalent transformation would be:

class Derived extends Base {
  constructor() {
    super();
    // mimics semantics of CreateDataPropertyOrThrow:
    if (!Reflect.defineProperty(this, "x", { value: 1, enumerable: true, configurable: true, writable: true })) {
      throw new TypeError();
    }
  }
}
mbrowne commented 5 years ago

This issue seems to be overlooking a simple fact: public fields are already proven to a large degree. They are already in wide use via Babel and also TypeScript (I don’t know all the TS details but it seems to me like TS public fields work pretty much the same way), and from what I’ve observed they’re being used quite successfully, including at the company where I work FWIW.

I think the real argument against public and private fields is this: https://github.com/zenparsing/js-classes-1.1/blob/master/docs/why-not-fields.md

The more problematic issues mentioned in the above link have to do mainly with private fields, not public fields on their own.

And I don’t think conflating the question of properties vs. fields with prototype vs. instance is helpful.

But just as an aside, I will mention one case where the late creation of instance fields can be an issue: #36. But since then my opinion has changed and I have been pretty satisfied with decorators as a workaround for this issue. For example, the whole class can have an @entity decorator that has access to all the class fields via the finisher function.

rbuckton commented 5 years ago

I don’t know all the TS details but it seems to me like TS public fields work pretty much the same way

TypeScript currently does a Set instead of CreateDataPropertyOrThrow. While we might consider a flag for consistency with the proposed semantics down the line, we will always support doing a Set as we transpile down to ES3 which does not support Object.defineProperty (but also doesn't support accessors).

rdking commented 5 years ago

I get that much, but the question is why? The standing precedent in ES, even for constructor-instantiated properties is to check the prototype tree first. Violating that seems, well, in bad form. Is there any particular justification for doing it this way?

rbuckton commented 5 years ago

The standing precedent in ES, even for constructor-instantiated properties is to check the prototype tree first.

Can you give an example? I would say the opposite tends to be the case. A lot of ES "classes" looked more like this:

// ES5
function Point(x, y) {
  this.x = x;
  this.y = y;
}
Point.prototype.offset = function (dx, dy) {
  this.x += dx;
  this.y += dy;
}

// ES2015
class Point {
  constructor(x, y) {
    this.x = x;
    this.y = y;
  }
  offset(dx, dy) {
    this.x += dx;
    this.y += dy;
  }
}
rdking commented 5 years ago

Try this one:

class BaseTest {
  get x() {
    this._x = (this._x !== undefined) ? this._x : Math.random();
    return this._x;
  }
  set x(v) {
    if (v !== undefined) {
      this._x = `You tried to set "${v}".`;
    }
    else {
      this._x = "It didn't work!";
    }
  }
}

class Test extends BaseTest {
  constructor() {
    super()
    this.x = "Let's see what happens";
  }
}

var a = new BaseTest();
console.log(`a.x = ${a.x}`);
console.log(`a._x = ${a._x}`);

var b = new Test();
console.log(`b.x = ${b.x}`);
console.log(`b._x = ${b._x}`);

The result is that the attempt by the derived class to set x is intercepted by the base class. What the current proposal intends to do will break this.

rdking commented 5 years ago

Unless I'm horribly mistaken (wouldn't be the first time), the order of ops is:

  1. check for an own property
  2. check for a prototype property
  3. repeat 2 until the protoype is null
  4. if found, call [[Set]] on the object where the property was found and return
  5. if not found, call [[Set]] on the instance and return
rbuckton commented 5 years ago

The result is that the attempt by the derived class to set x is intercepted by the base class. What the current proposal intends to do will break this.

No, the proposed semantics won't break your above code. What would break your above code is if you added the line:

class Test extends BaseTest {
  x; // <-- added line
  constructor() {
    super();
    this.x = "Let's see what happens";
  }
}

In your example, you don't have a declaration for x in your subclass, you assign (via Set) to the existing x. Declaring x in your class as a field means "I want my own declaration of x". You are welcome to leave it off.

Consider this non-class example:

const Test = Object.create(BaseTest.prototype, {
  x: { value: "Let's see what happens", writable: true, configurable: true, enumerable: true }
});

This also won't trigger your setter, and is another commonly used (thus idiomatic) ECMAScript pattern in use today without fields.

rdking commented 5 years ago

@rbuckton Sorry about that. I guess I wasn't as clear as I intended. Since the common explanation I was given for class { x=1; } is that the assignment is moved to the constructor, I'm thinking that without considerably more explanation, most developers would think that the code I wrote was an ES6 version of a public field declaration in the current proposal. That, of course, leads to bad expectations.

As for your other example, the developer has taken the time to explicitly override the property. This is not something that someone would expect from an x=y; type of variable setting. If it were instead a get x()/set x(v) pair, then it would make sense and fit the expectation. The get/set keywords are what users of class use when they want to re-define a property. The current proposal violates this expectation.

hax commented 5 years ago

@rbuckton

How do they differ?

In my memory, C# use reverse order for field initialization. But I have not used C# in recent years, I may wrong.

hax commented 5 years ago

@mbrowne

I’m also against this:

class Superclass {
  x = 1
}
class Subclass extends Superclass {
  x
}

Actually, in this case, at least it won't change the behavior, except x now undefined in Subclass (TS recently move to this semantic to align with this proposal, unfortunately), this is also likely unintentional.

And, in any time, Superclass may refactor to getter/setter which they believe should never breaking change, but actually ... boom.

hax commented 5 years ago

@bakkot

It creates an own data property shadowing the accessors, as I think it should,

It should only in your "field proposal" way, but it shouldn't in real world engineering. No use case. Just traps.

since that's what it looks to me like it's declaring.

Why x = 1 necessarily looks like declaration not assignment?

Use your own sentence again:

Otherwise the familiarity of the ${A} will mislead people into typing the familiar form of ${B}.

And I repeat several times today, Babel/TS use assignment semantic by default in all past years.

hax commented 5 years ago

@bakkot

if you would say that the current behavior is not what you want, rather than that the current behavior is improper.

If you are easily be trapped by a behavior which has no real world use case, I would say this behavior is improper, even it's proper according to the standard which provide such behavior.