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

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

Open
puppy0cam opened this issue Sep 14, 2019 · 14 comments
Open

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

puppy0cam opened this issue Sep 14, 2019 · 14 comments

Comments

@puppy0cam
Copy link

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

ljharb commented Sep 14, 2019

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

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

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

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

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

ljharb commented Sep 21, 2019

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

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

rdking commented Sep 21, 2019

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

ljharb commented Sep 21, 2019

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

@rdking
Copy link

rdking commented Sep 22, 2019

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

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

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

a-ejs commented Sep 22, 2019

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

rdking commented Sep 23, 2019

@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.

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

6 participants