-
Notifications
You must be signed in to change notification settings - Fork 27
Example of the utility of static private methods #4
Comments
@littledan not to me personally - i'd use "Extract into functions at module-level" every time - but I think the OP is a well-motivated justification for static private (the first I've seen). |
As shown in the second half, that doesn't really work if you need access to private state. |
Very true; i was trying to come up with an example I’d prefer, but the access to instance private state is the kicker. |
Seems like a key aspect here, which I missed in explaining the feature in the past, is that it's especially likely to come up with factory methods. I think there's a not quite exactly so bad way to split it up, by using instance private methods for the "second half": export const registry = new JSDOMRegistry();
export class JSDOM {
#createdBy;
#registerWithRegistry(registry) {
// ... elided ...
}
#init(factoryName) {
this.#createdBy = factoryName;
this.#registerWithRegistry(registry);
return this;
}
async static fromURL(url, options = {}) {
normalizeFromURLOptions(options);
normalizeOptions(options);
const body = await getBodyFromURL(url);
return new JSDOM(body, options).#init("fromURL");
}
static fromFile(filename, options = {}) {
normalizeOptions(options);
const body = await getBodyFromFilename(filename);
return new JSDOM(body, options).#init("fromFile");
}
} Yes, this is splitting in two, and if you're refactoring between public and private static factory methods, it would be not so much fun. Maybe there are other cases which split up even worse, and maybe it's unintuitive how to do the refactoring I did above, but any case I can think of divides up into two pieces more or less--before you create the instance (function outside the class) and after (instance private method). The final code, in this particular case, doesn't look so bad to me. Does anyone have any use cases for private static fields? |
Only personal-preference ones, analogous to the (1) vs. (2) from the first half of my post. |
I agree that this is a compelling reason to support them! Thanks, domenic :) |
I favor the variant of (2) that @allenwb suggested: support lexical declarations at the class level. This is like extracting to a function at module level, except that it is in the scope of the private instance field names, and can therefore access that private state on its arguments. What ever happened to that suggestion? Did anyone ever turn it into a proposal? Did it ever get disqualified for some reason? Why? |
I would be happy with such a variant. Especially if the way you declared such lexical declarations was by using |
@erights to clarify: static publics, and instance publics and privates, would continue as proposed, but static privates would be covered by lexical declarations (presumably with var/let/const) at the class level? |
Wasn't the declaration |
@erights My concern (@wycats convinced me of this) with @allenwb 's proposal: Keywords like I like what @domenic is suggesting: using me of syntax for a lyrically scoped function or variable. I presented something like this initially as the semantics for both instance and static private methods (not my idea; it was derived from earlier comments from @allenwb and others. See this patch which switched to doing type checks). Several committee members said in issues (#1 #3) that would prefer type checks on the receiver rather than the lexically scoped semantics. If we did that sort of change, there are some semantic details that I'm unsure of. Should we revisit these semantics for just static private fields and methods, but leave static private methods actually on the instance? What would we do with the receiver--just pass it as the receiver of the function, and entirely ignore it for static private fields? |
@domenic, strictly speaking you can still do what you want without requiring all that overhead: export const registry = new JSDOMRegistry();
let finalizeFactoryCreated;
export class JSDOM {
#createdBy;
#registerWithRegistry() {
// ... elided ...
}
async static fromURL(url, options = {}) {
normalizeFromURLOptions(options);
normalizeOptions(options);
const body = await getBodyFromURL(url);
return finalizeFactoryCreated(new JSDOM(body, options), "fromURL");
}
static fromFile(filename, options = {}) {
normalizeOptions(options);
const body = await getBodyFromFilename(filename);
return finalizeFactoryCreated(new JSDOM(body, options), "fromFile");
}
static finalizeFactoryCreated(jsdom, factoryName) {
jsdom.#createdBy = factoryName;
jsdom.#registerWithRegistry(registry);
return jsdom;
}
}
finalizeFactoryCreated = JSDOM.finalizeFactoryCreated;
delete JSDOM.finalizeFactoryCreated; But I agree this is significantly less clean.
Edit: sorry, that that bit is false of course, since you've written Edit 2: the second part of the edit was also false. :| Note to self: I should not try to remember how languages work when running a fever. None of our options seem all that great. |
(Replying to the edit.) I'd be very surprised if people used |
Now that I look, in fact you can't. Don't know what I was thinking, sorry. |
@bakkot I don't think we should be thinking about delete as a viable option of a replacement. Without that, do you have other thoughts on whether private static methods are significantly motivated? |
I've updated the explainer to advocate for the original solution, which does include static private methods, based on the feedback in this thread that they are important. Please keep the feedback coming! |
@littledan I expect that pattern could be captured by a decorator without requiring any
This issue gives a good motivation, I think, but I'm still worried about the subclass footgun. I think it actually is pretty likely to come up in real code if we go with the current semantics. I'll riff off @domenic's example above (the 'code I want to write' option). Suppose that we the implementors of the export const registry = new JSDOMRegistry();
export class JSDOM {
#createdBy;
#registerWithRegistry() {
// ... elided ...
}
async static getBodyFromURL(url) {
// ... elided ...
}
async static getBodyFromFilename(filename) {
// ... elided ...
}
async static fromURL(url, options = {}) {
normalizeFromURLOptions(options);
normalizeOptions(options);
const body = await this.getBodyFromURL(url);
return this.#finalizeFactoryCreated(new this(body, options), "fromURL");
}
static fromFile(filename, options = {}) {
normalizeOptions(options);
const body = await this.getBodyFromFilename(filename);
return this.#finalizeFactoryCreated(new this(body, options), "fromFile");
}
static #finalizeFactoryCreated(jsdom, factoryName) {
jsdom.#createdBy = factoryName;
jsdom.#registerWithRegistry(registry);
return jsdom;
}
} // elsewhere:
import { JSDOM } from "jsdom";
class SpecializedJSDOM extends JSDOM {
static getBodyFromFilename(filename) {
// ... some different implementation ...
}
static fromFile(filename, options = { something }) {
return super.fromFile(filename, options);
}
serialize() {
// ... some different implementation ...
}
} (diff)
--- domenic.js
+++ me.js
@@ -6,20 +6,28 @@
#registerWithRegistry() {
// ... elided ...
}
+
+ async static getBodyFromURL(url) {
+ // ... elided ...
+ }
+
+ async static getBodyFromFilename(filename) {
+ // ... elided ...
+ }
async static fromURL(url, options = {}) {
normalizeFromURLOptions(options);
normalizeOptions(options);
- const body = await getBodyFromURL(url);
- return JSDOM.#finalizeFactoryCreated(new JSDOM(body, options), "fromURL");
+ const body = await this.getBodyFromURL(url);
+ return this.#finalizeFactoryCreated(new this(body, options), "fromURL");
}
static fromFile(filename, options = {}) {
normalizeOptions(options);
- const body = await getBodyFromFilename(filename);
- return JSDOM.#finalizeFactoryCreated(new JSDOM(body, options), "fromFile");
+ const body = await this.getBodyFromFilename(filename);
+ return this.#finalizeFactoryCreated(new this(body, options), "fromFile");
}
static #finalizeFactoryCreated(jsdom, factoryName) {
@@ -28,3 +36,24 @@
return jsdom;
}
} Unfortunately, this doesn't work: trying to call So while it's true that, as the readme explains, that linters and good error messages and so on might be able to discourage the use of |
As an alternative @bakkot and I have discussed installing private static methods on subclasses, but omitting private static fields. Do you all have any thoughts on that approach? |
To add on to the above: come to think of it, there's a case where using Suppose that export class JSDOM {
// ...
static #finalizeFactoryCreated(jsdom, factoryName) {
if (typeof this.finalize === 'function') {
this.finalize(jsdom, factoryName);
}
// ...
}
} If you can use This sort of thing is why I am in favor of the approach @littledan mentions. |
I am very skeptical of this hypothesizing about desiring some sort of polymorphic dispatch on static members, and especially private static members. I don't believe any other language supports that, and I don't think we should be adding machinery to JS (such as copying the methods to subclasses) in order to support that use case. My mental model of statics is essentially that they are lexical declarations at class level. I realize for public static you can do polymorphic dispatch, but I am fine saying that is not true for private statics, especially given that private instance fields are already very anti-polymorphic by virtue of not participating in the prototype chain. |
@domenic I'm not so convinced by the "code grouping" requirement. There's lots of things that we don't allow grouped, e.g., class declarations in the top level of a class (you could do this through static private fields but it would look weird). I think it should be OK to put things that are not exported outside the class declaration; I don't think initializer expressions are likely to contain something that needs to reference private fields or methods. One reason that supporting private static methods on subclasses isn't all that ad-hoc is that it's actually sort of analogous to instance private methods--it's installed on all the objects that it would've otherwise be found on the prototype chain. Does this make @bakkot 's proposal any less excessive-feeling to you? About polymorphic dispatch on static members: in another thread (having trouble finding it now), @allenwb explained that ES6's practice of assembling a prototype chain of constructors is deliberate, useful and matches other dynamic object-oriented languages. About the complexity of private fields in JS and the implications of that: It's true that this is relatively new ground, largely because other dynamic languages do not attempt to enforce privacy to this extent (but we have good reasons for making the choice we did). If the mental model of statics is that they're lexical function declarations, then this would allow for the genericity in @bakkot 's comment. If you really want lexical semantics, we could just straight-up say that private static fields are just lexically scoped variables and private static methods are just lexically scoped functions (all with funny method/field-like syntax). Would anyone prefer that? |
I'd guessing you're thinking of this comment. |
tldr: I'll change the semantics to be, you can use Following some discussion on #tc39 with @bakkot, @domenic and @ljharb , I'm leaning towards a new intermediate option: Private static methods are installed on subclasses, but private static fields are not and still have a TypeError when read or written to subclasses. I'll edit the explainer to give more detailed reasoning and write spec text specifying this option, but to summarize:
|
@littledan I quite like this outcome. It aligns class initialization more closely with instance initialization where it can be done without confusing side effects, and disallows cases where side-effects make all of the options compromised. At the same time, the use-cases for static private fields can virtually always be accomplished using a lexical variable, which was not the case for static private methods. TLDR: Between really needing static private methods semantically and the fact that they aren't confusing due to side effects, there's a strong argument for making them work. Inversely, between not really needing static private fields semantically and the fact that they are confusing due to side effects, there's a strong argument for disallowing them. I like this conclusion. |
@littledan to clarify, for static private methods you can use (where |
@ljharb Exactly, thanks for the clarification. I fixed the wording as you suggested. |
@wycats To make sure we're on the same page, the proposal is not to disallow private static fields, but rather to allow them with the semantics that referring to |
Ah I didn't understand that, so thanks for the clarification. I think, for me, the consequence of this decision is that I would avoid static private fields and teach people to avoid them. Was there a strong argument for static private fields that made us want to give them these semantics rather than defer them? Either way, I could live with this conclusion. |
I agree.
I strongly agree.
I wouldn't say no issues, but everyone (including me) seems to be comfortable with normal Javascript prototype behavior (including copy-on-write semantics) if static private fields are taken off the table.
Strange situations are certainly possible, but I agree that they don't really feel like new issues. class A { #private = "A"; getter() { return this.#private; } }
class B { #private = "B"; getter() { return this.#private; } }
var traitor = Object.setPrototypeOf(new A(), B.prototype);
A.prototype.getter.call(traitor); // => "A"
traitor.getter(); // => TypeError
I've been persuaded. It's extremely unfortunate that the class fields proposal introduces a new "private" concept that doesn't apply to any other class feature, but not technically fatal. The table would end up like this:
|
@allenwb I agree with @bakkot's argument in #4 (comment) . To clarify where we are in committee discussion: I brought the question about whether it's appropriate to pursue private methods given the lexically scoped function alternative possibility, to TC39 twice: in the July 2017 and September 2017 TC39 meetings. We discussed multiple alternatives, including function declarations in class bodies, private method syntax with lexical function declaration semantics, and private method syntax with private field semantics. The committee reached consensus in July on the third alternative. With this option, private methods reached Stage 3 in the September 2017 meeting. Later, in the November 2017 meeting, we confirmed that private instance methods are at Stage 3. I believe Allen's arguments in favor of lexically scoped function declarations instead of private methods were made well-known to the committee. I tried to present them in my private methods presentations, but Allen had also made them directly to the committee in the past. So, I'm not sure what new information we have to revisit these decisions. @gibson042 About using lexical scoping rather than |
The difficulty I keep having with @allenwb 's lexical scoping proposal is that it seems odd to me to have a bare function declaration or let declaration in the middle of a class body. I was discussing this with @ljharb and he had an interesting idea for what the syntax could be: class C {
static function x(v) { return v+y }
static let y = 1;
method() { return x(1); }
}
new C().method(); // => 2 What do you think? We could use a different keyword instead of |
@littledan At first glance I like this direction! And we already have precedent to prefix declarations with a keyword, for module exports. |
Note as well; my suggestion wouldn't necessarily preclude |
class C {
class function x(v) { return v+y }
class let y = 1;
method() { return x(1); }
}
new C().method(); // => 2 It explicitly says: I am defining a class (scoped) function or variable.
This seems symmetric with the odd feeling I get when I see what looks like an assignment statement (ie, a instance field declaration with an initializer) in the middle of a class body. I suspect we would both would get over those feelings after enough usage. One of the arguments for the keywordless field definitions was that the keyword is unnecessary noise and that developers will be annoyed by the extra typing. The same argument seems applicable here. But, I even with a redundant keyword, I think this would be much better than what has been kicked around for private methods and static private fields. |
Let me make a suggestion for a better way to think about static # inheritance. Unlike class instances, class constructors are singleton objects. Static fields (properties or private slots) are defined directly upon the constructor objects and constructor inheritance chains go directly from subclass constructor to superclass constructor without going through intermediate prototype objects. Constructor inheritance is pure prototypal inheritance. There is one other context in JS where we declarative define single objects without also defining an associated prototype -- Object literals. To me, the semantics of For example: let obj1 = {
#ctr: 0,
get counter() {return this.#ctr},
increment() {++this.#ctr}
};
let sub = {
__proto__: obj1
};
console.log(obj1.counter); //0
sub.increment();
console.log(obj1.counter); //1 I really think we should steer away from private static fields (at least for now) but if you want think about them, think about object literal singletons. |
The semantics you describe make sense to me, ftr. |
Also i think |
@littledan I really love this direction. One thing that's been gnawing at me this entire time (and which @allenwb has repeatedly pointed out) is that all static private features are easily modellable using normal closures. And since private static methods are lexical, they're really just a convoluted way to describe a lexically scoped function. The only reason that isn't sufficient to make that the end of the story here is that we need the functions to be on the inside of the class body in order to have access to instance state defined inside the class body. @allenwb is correct that allowing lexically scoped functions inside the class body addresses the use cases for static private fields with reasonable ergonomics and a clear programming model. The issue with putting a bare @littledan's proposal to use a prefix modifier like TL;DR I would love to see us decouple static/private stuff from this proposal and focus our efforts on class functions for those use cases for now. |
Bikeshedding: One thing I liked about @ljharb's suggestion of
Note that we're not restricted to existing keywords here and can use basically anything as a contextual keyword (as long as we're ok with the no-newline-after limitation). |
I'm not sure I agree entirely with this phrasing of the problem space. I think there are two reasons programmers may put code in a method:
The private instance methods proposal opts to use method syntax, rather than lexically scoped function syntax, to retain consistency and parallelism for the second case. Where a method uses
This seems like a really useful lens; thanks for bringing it up. The original proposal for class fields would throw a TypeError in both cases and be consistent with respect to subclassing. (I actually wrote out a sketch of what object literal semantics would be in an attempt to create consistency here; however, I went back and removed it because it seemed too limiting to have object private fields scoped only to a single literal.) However, the current draft spec, due to inheriting private static methods and accessors, loses this property: Private static methods are inherited (copied) by |
@bakkot and @rbuckton both separately brought up another possible syntax: using a class C {
static {
function x(v) { return v+y }
let y = 1;
}
method() { return x(1); }
}
new C().method(); // => 2 What do you think of this? (emoji reacts welcome) Advantages:
Disadvantages:
|
I think this is a pretty large con. Being able to reference declarations that are defined inside an inner curly brace feels really bad. |
A bit behind here...
Not precisely. As you know, it's quite hard to get good data here, which is why we lean on educators and other people with exposure to broad parts of the community. On this particular question, though, I think every single person I've ever talked to about it had the expectation that they'd be per-instance. Here's an example from the private fields discussion, which a lot of people apparently felt was reasonable despite there being no way it could possibly work without fundamentally changing what closures are; later someone says they tried using lexical declarations in class bodies in exactly this way (because an older JS pattern) and were disappointed when it didn't work.
I actually don't think this is attributable to experience in other languages. One reason is that there's been a JavaScript pattern taught for many years which suggests using variable declarations in constructors to create (per instance) "private members": here's Crockford giving it, for example. And yes, that's not the situation here because the methods in a class body are shared, rather than being created every time the constructor runs, but especially given the examples I've linked I'm not convinced the instinctive mental model of lay programmers makes that distinction.
I'm not sold on this particular claim. Because of the type check, the semantics are equivalent to "define immutable field on objects which are instances of this class, with a name which no other objects can share or reference", and not equivalent to "define a lexically-resolved function". I also agree with @littledan's reading of how people tend to think about functions vs methods. As an example, if I have an object
This would be confusing for me personally, but I think I like it better than I think of these my preference is for |
OK, it seems like we have the shape of a possible solution here:
The next steps from here would be:
Although the above proposal is significantly different in semantics, it's an iteration on the same problem space, so I don't think it requires the process overhead of a new staged proposal. |
I'm developing an alternative explainer and spec text based on lexical declarations in #10 . |
@littledan I agree with your analysis, but especially with the point that private instance methods are still justified. |
Draft spec text for adding lexically scoped declarations; see discussion of semantics in the commit message. Better explainer text coming soon. Any feedback would be very appreciated. 4a2d164 |
We have concluded that private static will be part of this proposal, and it has advanced to Stage 3. |
This is loosely based on existing code in jsdom, with some additions to illustrate further points.
I am working on a class, call it
JSDOM
, which has some static factory methods:I notice two things:
Both of these are things that are addressed well by the extract method refactoring.
I have two choices on how to do this:
Although it is a matter of opinion, I find (1) better than (2), because it keeps the code inside the class that it's designed for, and near the methods that use it, instead of somewhere later in the file. Today, (1) has the disadvantage of polluting my public API, so I usually choose (2). But if we had static private methods, I could do (1) without that drawback.
Still, let's assume that arguments from #1 prevail and we don't get static private methods. It's not too bad:
But as I continue my work on implementing these functions, the lack of static private ends up felt more deeply. I get to the following point:
Again I notice I have some duplicated code. Here is the code I want to write to clean this up:
But I can't, because static private methods don't exist. Here is the code I have to write to work around that:
This code is basically worse than the original code with duplication. The lack of static private, and the workarounds I have been forced to employ to get around it, have made persisting with duplication a better choice than attempting to apply extract method---because extract method has basically been broken.
This is why I think static private methods are quite important. Without it you cannot apply basic refactoring patterns, which users will expect to be feasible, to JS classes.
The text was updated successfully, but these errors were encountered: