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

Assigning a function to a private field isn't ideal #270

Open puppy0cam opened 5 years ago

puppy0cam commented 5 years ago

When attempting to assign a function to a private field, it creates a new instance of the function for every definition. This is not ideal, and means you are unnecessarily creating a new function object on every instance of the class. I have made the following code for this dilemma:

class A {
    #doesMeetRequirements = function() {
        return this.a && !this.#b;
    }
    #b;
    a = 0;
    constructor(value) {
        this.#b = value;
    }
    checkState() {
        if (this.#doesMeetRequirements()) {
            return this.#b;
        } else {
            throw new Error("The requirements have not been met!");
        }
    }
}

I do not want to have to declare the function for every instance of the class, and I cannot work around it as the function MUST be created within the class body. One might think that they could work around it by declaring the function outside the class body, and just adding it to the prototype, however this is not allowed.

class A {
    #doesMeetRequirements; // I get my value from the prototype.
    #b = 1;
    a = 0;
    constructor(value) {
        this.#b = value;
    }
    checkState() {
        if (this.#doesMeetRequirements()) {
            return this.#b;
        } else {
            throw new Error("The requirements have not been met!");
        }
    }
}
A.prototype.#doesMeetRequirements = function() {
    return this.a && !this.#b;
}

The other option is to create a private static field containing the private method's function which you assign as the value when the class is initialised.

class A {
    static #doesMeetRequirements = function() {
        return this.a && !this.#b;
    }
    #doesMeetRequirements = A.#doesMeetRequirements;
    #b = 1;
    a = 0;
    constructor(value) {
        this.#b = value;
    }
    checkState() {
        if (this.#doesMeetRequirements()) {
            return this.#b;
        } else {
            throw new Error("The requirements have not been met!");
        }
    }
}

The problem with this approach is that you cannot have a static class field with the same name as an instance's class field. Finally, you change the name of the static field to some random string of letters and numbers. But when you have an error thrown from the private method, you have no idea what function it is referring to as you obfuscated the name

Thrown:
Error
    at A.#hasfuihfaso (repl:3:15)
    at A.checkState (repl:13:39)

Finally, you remember that underscores are a thing and you end up working it out in the end. But you really shouldn't have had to go through so many hoops to make the private method work properly.

I would like to suggest that you should be able to access private fields of the class prototype from the same script as the class declaration. Currently, if the value is not given an initial value, it will default to undefined but still be declared on the created object. I would like to be able to set it's default state from the prototype if there is no default state defined by the class declaration.

ljharb commented 5 years ago

This is true of public fields as well, where it impacts testability.

However, apparently v8, at least, optimizes this in public fields so that it’s not as expensive as creating N functions, and i assume that similarly performance will be a non-issue with functions in private fields too.

littledan commented 5 years ago

Interesting code pattern. I'm wondering, would private methods work for your use case? They are not supported yet by V8, but you can use them in Babel, and V8 support is on the way.

puppy0cam commented 5 years ago

I was under the impression that private methods were going to come with another proposal down the chain, but I often like to work with prototype chains anyways.

littledan commented 5 years ago

Neither private methods nor private fields involve the prototype chain. Private methods involve reusing the same function, as makes sense for this case. Private methods are at Stage 3 and several implementations are available or in progress. It's important to evaluate the initializer for each instance in case it's something like [], to avoid bugs that would otherwise be caused. For these reasons, I'd conclude that we don't need to make changes to the class fields proposal for this use case.

puppy0cam commented 5 years ago

While functions were the primary concern in this issue, I would also like to mention that some may find it confusing that they can access new.target in the constructor function, but not in a class field's initialiser.

class MyDate {
    static DATE_OFFSET; // classes extending me should set this value!
    #offset = new.target.DATE_OFFSET; // Cannot read property 'DATE_OFFSET' of undefined
    #constructed_at = Date.now();
    constructor() {
        this.#offset = new.target.DATE_OFFSET; // this works, but would make more sense as an initialiser.
    }
    getOffsetConstructionDate() {
        return new Date(this.#constructed_at + this.#offset);
    }
}
ljharb commented 5 years ago

You also can’t access constructor arguments in an initializer - because it’s not the constructor, it just runs almost as if it were in the constructor.

littledan commented 5 years ago

We had a really extensive discussion about the scope of initializers. In the end, they are designed to be similar to method scopes. This makes sense, as they are not contained in the constructor and they are at the same syntactic level as methods. This logic implies that new.target is undefined.

rdking commented 5 years ago

The only problem is that even though TC39 has had "really extensive discussion" about it, the intuition of developers will invariably differ from what you've discussed. In whatever literature and/or training is posted about this feature, it needs to be made clear that each initializer is essentially a separate function being called during the execution of the constructor and not an assignment being made directly within the constructor's scope.

There's just nothing about the syntax of these initializers that even hints that they're being boxed in a function (although most people will understand that this was necessary for dealing with initializing objects).

ljharb commented 5 years ago

The intuition of some developers will differ; that’s true of any decision made.

rdking commented 5 years ago

Please don't obfuscate the meaning of what I said. I'm saying that developers who haven't read and understood the fine details of this proposal are likely to think that these initializers are supposed to be run directly in the scope of the constructor because nothing in their syntax makes it even remotely evident that it would be better to consider each initializer as a separate function called from the constructor's scope.

nicolo-ribaudo commented 5 years ago

I think that it's clear: class fields are outside of the constructor's braces. Currently things inside {} are not accessible from the outside (except for hoisted vars inside blocks)

puppy0cam commented 5 years ago

When you are teaching developers how to use class fields, their example for comparison to previous functionality might look like this:

class A {
    constructor() {
        this.value = (function() {
            return new.target; // undefined
        })();
    }
}
Function new.target available?
super class fields no
super constructor yes
constructor class fields no
constructor yes

But I think that this example would be more intuitive if it were more like an arrow function.

class A {
    constructor() {
        this.value = (() => new.target)(); // [Function: A]
    }
}

Or even just setting the property directly!

class A {
    constructor() {
        this.value = new.target; // [Function: A]
    }
}

Both of the ideal examples would give access to new.target at all stages.

a-ejs commented 5 years ago

@nicolo-ribaudo Well, you could also say that 'currently' all statements in any code are evaluated (roughly saying) one after other, with only exception being functions: code inside function bodies is executed when they're called. Yet class fields syntax violates that: it mixes 'on-the-run' definitions (all methods and static fields are evaluated & assigned right away) and 'function code' (instance fields) in one syntactic entity: the class body. I'd say that this breaks existing patterns much more severely than violation of 'things inside braces aren't visible from outside them' would.


I personally do not understand the need to abstract from existing prototype-based inheritance in JS. 'Class fields are not part of the constructor', yet 'classes' in JS are just constructors and prototypes. Constructors initialize objects and prototypes describe their common properties (methods). Why does this model need to be hidden behind this vague syntax?

rdking commented 5 years ago

@nicolo-ribaudo

I think that it's clear: class fields are outside of the constructor's braces. Currently things inside {} are not accessible from the outside (except for hoisted vars inside blocks)

This proposal adds new exceptions, like the use of this(referring to the instance currently being created) in an initializer. That's something that only exists within the curly braces of a function. A class declaration is not a function. If you're going to apply such intuition, you should do so evenly.