Why are underscore prefixes for internal methods of a React component considered bad?

[ttmarek]

The third bullet under react#methods says:

Do not use underscore prefix for internal methods of a React component.

I was just wondering what the reasoning behind this was?

[lencioni]

They give people a false sense of "private"ness that could lead to bugs.

[ttmarek]

Okay. Even though JavaScript doesn't support private variables, I still think the underscores help communicate the programmer's intent. If possible, @lencioni could you give an example of this false sense of "private"ness that lead to a bug? Or maybe a scenario where the underscores would confuse another programmer and lead them astray. I'm not disagreeing with you, I just want to have a better understanding of how this rule came about.

[ljharb]

Private means inaccessible. Your intent to privacy is irrelevant if the value is reachable, ie public.

For example, npm broke node once by removing an underscore-prefixed variable.

[ttmarek]

npm broke node once by removing an underscore-prefixed variable

If the variable wasn't prefixed they would have used it either way, and node would have still broke. When a meant-to-be-private variable is prefixed with an underscore there is at least one line of defence (a weak one, granted) preventing its use. Whereas when a meant-to-be-private variable isn't prefixed with an underscore, there is no line of defence (not even a weak one).

[ljharb]

@ttmarek yes, but had they not prefixed it with an underscore, nobody would have been fooled into thinking it was actually private, and there would have been test coverage, and changing it would have prompted a semver-major version bump.

An underscore is in no way, whatsoever, a line of defense - it's just an incorrect assumption some developers make about a language that simply does not have the concept of privacy outside of closures.

[ttmarek]

@ljharb So, in other words: underscore prefixed methods might confuse programmers from other languages into thinking that JavaScript supports private methods. Right?

[ljharb]

That, or programmers who think that convention is the same as privacy.

[ttmarek]

Okay, cool. Thanks @ljharb, I see what you're getting at. Do you think its worth adding a short blurb about this under the third bullet in react#methods?

[lencioni]

I think so. I'd be happy to merge a PR that does that.

[ljharb]

Sure, more helpful info is always good.

[ericelliott]

My favorite reason:

Private stuff tends to change without warning, and without advertising as a breaking change.

The problem with that is that a lot of code could break without warning, because EVERYBODY ignores underscores:

Newbies don't know what it means. Experienced developers think they know what they're doing and the underscore doesn't apply to them.

Admit it. You've done that, too.

[mpj]

I think @lencioni is perfectly correct by saying

They give people a false sense of "private"ness that could lead to bugs.

... but I'd like to elaborate on why.

In programming languages in general, the point of having a private property (as opposed to having all properties public) is so that you can trust a private variable not to be changed by outside code, unlike public properties which you need to assume can be changed by outside code at any time.

If you're absolutely hell bent on using inheritance in JavaScript, you don't have private properties. It is not a feature of inheritance in JavaScript. Everything is public. The logical thing to do is therefore to treat every property as a public property (because it is) - your code needs to assume that it can change at any time.

You can call a property _myProperty or __DONTFUCKINGCHANGEmyProperty but that doesn't make that property private. It's public. Private does not exist in the object composition model that you have chosen. You're trying to reap the benefits of a feature that you do not have. If you write your code in a way that assumes that your public-property-with-special-name-convetion does not change, you're just fooling yourself. The fucker can change at any moment, just like any other of your properties.

(Sidenote: This is React, and the following is not an option there, but in general, unless you need to create more than 50000 items per second, I personally think that you should consider using factory functions (https://www.youtube.com/watch?v=ImwrezYhw4w) instead of inheritance for creating objects. They have real privacy and do not have this problem)

[gabro]

Hi everyone, I've landed here after searching for an ESLint rule for enforcing this convention. Since I haven't found any, I've written one myself. In case anyone is interested, here it is: https://github.com/buildo/eslint-plugin-method-names

[ljharb]

@gabro there's no-underscore-dangle which prevents it on identifiers - it seems like that would be a useful addition to the core rule, rather than a separate plugin. You should file an issue requesting it on eslint itself :-)

[gabro]

@ljharb yes, I will. Not sure whether to restrain the scope to underscores or to keep it generic as it's now, but I guess we'll figure it out in the ESLint issue.

[gabro]

@ljharb FYI here's the proposal eslint/eslint#7065

[StokeMasterJack]

I actually don't agree with this. I think underscore is a good way to indicate a private method. Even if the JavaScript runtime does not enforce it. However, given React's imperative style, it doesn't matter much. In 99% of the cases, most internal methods (other than the React callbacks) are typically private.

[ljharb]

It's not actually private if it's not enforced - as such, it constitutes either a mistake or a lie.

[QuentinRoy]

The underscore convention has been successfully used in the python world for years.
It is not a mistake. Everybody knows it does not prevent access to the variable (except absolute novices that are quickly undeceived).
And it is even less a lie (!!!!). Even though I am not too sure what it means to "lie" when coding, I can tell that nobody is pretending it enforces anything.
It is is a convention. Nothing more and nothing less. A convention telling: "This is internal, you can access it, but this is not supported and you'll do it at your own risk."
You can like this convention or not, there are pros and cons and I don't think the point is to discuss it here, but don't call a whole community "lier" or "stupid".

[ljharb]

Indeed, it is a convention - one that causes confusion by implying privacy where none exists. There are certainly many ecosystems that have used convention as a boundary, but in the JS ecosystem, we've learned that convention is not strong enough of a boundary. If it is possible, people will do it, and any damage caused is the fault of the software author - never of its users. "Use at your own risk" is a reckless and hostile thing to claim as a software author - I, and this guide, choose to instead treat our ethical obligation to our users with higher respect than that.

[QuentinRoy]

I believe this is subjective (so far nobody gave me any convincing arguments of the assessments like "not strong enough"). I don't think that I - and the others who disagree with these assessments – are being "reckless", "hostile" or disrespectful when we chose not to use workarounds to hide the internal state of our component to our consumers (I have not found a "private" JS pattern readable and handy).

Is it my responsibility if someone code is broken because he used a variable I told him he should not?
When you buy a electronic device, it is usually not recommended to open it. Is it Samsung or Sony's fault if you broke your laptop when trying to change welded chip? I don't think so, and they usually clearly state that doing such voids the warranty. You can do it if you want to hack it, but of course you may not complain if you broke it. Apple decided to enforce it a bit stronger by using non-standard screws. You may chose to do something similar.

The general argument is that internal state should be protected. To quote again the python community: "who are you protecting the attribute from?". I think that is just really a question of preference.

[ljharb]

Yes, as an author, it's your fault if your users are broken, even if they disobeyed your instructions. It's your obligation to protect them from themselves - the "we're all consenting adults here" philosophy doesn't really work.

[scarmuega]

This thread is very interesting, I think it goes beyond the actual "underscore" issue.

In my opinion, most of the open source world works by asuming that "we're all consenting adults".

SemVer is a weakly enforced convention (lib author decides when to change major or not) and yet packagers rely their whole dependency-update-logic on the assumptions that the developer was careful enough to follow the versoning rules.

I understand that everything is publicly accesible in JS, but that doesn't mean it has to be part of the public API of your library. IMHO, if a consumer is using a piece of code that doesn't appear in documentation and/or has an underscore prefix, then he should be treated as a consenting adult.

[QuentinRoy]

@scarmuega Just override the rule. This style guide is strongly opinionated, in particular when it comes to that topic. After some thoughts this is actually fine, we need Crockfords. You don't have to take the style guide entirely, and should not take it blankly.

[QuentinRoy]

I define "inappropriate" as "relies on a feature that does not exist in the language".

I think "relies on a feature that does not exist in the language" is a cause of inappropriateness, not a definition of it. If it is, then

fake [private fields] by other means demonstrates an inappropriate architecture for JS

is a bit tautological. The reason I am asking is because I am trying to understand the problem: what is the bad concrete thing that may happen if I use _ and why. I was looking for something like what @ericelliott gave a bit later.
Also again, I would like to emphasize that nobody is trying to fake anything and I think this is important. We are merely using a convention to indicate to consumers of our stuff that doing something, i.e. relying on the value of a particular label, is unsupported by our API.
@ericelliott, I entirely agree of course this increases the attack surface, but unless you're exposing an API to interact with your own infrastructure, attack from whom and against whom? If I build a slider and my consumer "attacks" the API by doing stuffs explicitly unsupported, he is attacking himself really.
I know @ljharb also disagrees with this, but I still consider that my responsibility is to be clear about how my thing is designed to be used, not to prevent people from "hacking it" in their own software (if they want to they will manage, that's open source anyway).

EVERYBODY ignores underscores

I have a hard time believing this. I mean this is false anyway because at least I do. What is the proportion of people who do not respects it, you know better than me, but I am humbly surprised it is so high. I understand people could do it because they don't know what it means. However:

Experienced developers think they know what they're doing and the underscore doesn't apply to them.

That looks like very dumb to me. You know this is unreliable and you still rely on it? You can blame me, but I will not take the responsibility if you put your cat in my microwave despite me telling you that you should not put cats inside my microwave. And I am still not planning to add cat sensors inside (at least not for this reason).

@ericelliott In my opinion it is less a question of consensus than awareness. What I mean is I don't care if my consumer agrees with me using _ or not. As long as he knows what it means, as far as I am concerned he can safely use my stuff and there is no problem. Where I think you do have a very good point though is that as I understand and from your (valuable) experience, it is not clear for many people in the JS community that _ means "don't touch me or you're on your own". I would have expected this population to be small enough to be negligible (maybe because I did python before JS) and that such mistakes would be more often that not caught during code reviews (that is easily spotted and eslint even has a rule for it), but that may be naive.

Also, thank you for reporting your experience. I have actually been looking for such examples. This indeed looks like a very bad case. You presented a lot the consequences of the issue, however little in how you determined the cause. If you are allowed to, could you flesh it out a bit more? Just to be clear I entirely trust you when you say _ was the cause, I just would like to understand why and to what extent. Where you accessing _values? Any idea how the codebase ended up with that in the first place?
My point in asking this is that I will take responsibility for young developers not knowing what they were doing—because as I said, the contract needs to be clear and it is possible that the meaning of _ is not clear for the JS community. However I will not take responsibility for experienced guys consciously breaking the contract because deadline or something else.

[omeid]

@ericelliott,

What lint rule simply forbids ._prop access?

The allowAfterThis option for no-underscore-dangel.

No -- it's private. There's no reason to describe it with JSDoc, and you should be doing black-box testing -- test only the public API, not the private implementation details, for the same reason it's unsafe to use private methods externally -- they're more likely to change and break than the official public API.

But all the other methods that internally depend on this private stuff needs to be documented though.

Again, no, because, while superficially it looks like I'm returning the store, the very thing we're trying to keep private, we could easily change the implementation or shape under the hood, update the implementation of getState(), and everything keeps magically working.

Well sure, in the updated example, but it still suffers from the issue that any method that depends on the internal state would have to be defined in the constructor, or use a method that exposes this internal state, which again, in turn leaks this internal state.

"Program to an interface, not an implementation." ~ Gang of Four, "Design Patterns"

Which means you should distinguish between implementation details and intentional APIs.

Limiting the API surface has the natural side-effect of limiting the attack surface. It's basically lesson 3 when you start to learn about hardening software against attacks, right after authentication and authorization.

What sequence of lesson are you speaking of? last time I checked, security was not packed into a single all-in-one course, it is a discipline with far and wide aspects and concerns.

Anyways, attack surface does not apply to object interfaces that is intended to provide separation of concern, relying on object encapsulation for security/secrecy is absurd, it is intended to provide visibility.

Further more, it is obvious that using underscore does not provide any mechanisms for visibility, and is merely a conventional contract around API stability, if you're writing secure applications without understanding the basic constructs of the language you're using, I will be concerned about the people who might rely on such work.

[ericelliott]

But all the other methods that internally depend on this private stuff needs to be documented though.

Yes, but you're documenting and testing only their public API, so you can safely ignore the details of the internal stuff. That's what black box means.

Well sure, in the updated example, but it still suffers from the issue that any method that depends on the internal state would have to be defined in the constructor,

Yes, they would be defined in the constructor. In JavaScript, we call those "privileged" methods, because they do have access to the internal state for their implementation details.

or use a method that exposes this internal state, which again, in turn leaks this internal state.

There is no need to do that, because the right way is to use privileged methods.

"Program to an interface, not an implementation." ~ Gang of Four, "Design Patterns"

Which means you should distinguish between implementation details and intentional APIs.

Yes.

And no. In OOP, the canonical way to "distinguish" between implementation details and public APIs is to encapsulate the implementation details so you can't get at them at all from the public API.

The core idea of OOP -- the very essence of what OOP is -- the concept that objects are encapsulated, and that they control access to internal implementation details and private state.

OOP = encapsulation + message passing (method calls, etc...) -- the rest is just icing to help us structure relationships between objects (facilitate encapsulation and message passing).

If you're not doing encapsulation, you're not doing OOP.

If you remove encapsulation from the list, you've lost the essence of why OOP exists in the first place.

Quoting Alan Kay now (the founder of OOP):

"Object-oriented programming to me means only messaging, encapsulating and hiding state, and extreme late-binding of all things." ~ Alan Kay source

More in-depth:

"My original thought was to have something like recursive biological cells. We have about 10 to the 14th power of cells in our body. That is a hell of a lot more cells than there are nodes on the Internet. Those cells spend almost all of their effort keeping themselves normal. They're self-repairing, and you don't have to stop the organism in order to affect repairs. And then there are some interesting mathematical properties of this kind of thing that also occurred to me, and I called those things objects."
"So I thought of objects being like biological cells, only able to communicate with messages (so messaging came at the very beginning - it took a while to see how to do messaging in a programming language efficiently enough to be useful)."

[ericelliott]

On Security & Encapsulation

@QuentinRoy

Lesson 3a in software security: Never assume you know who is attacking whom, what their goals are, or even that the attacker is actually the user in question. Your job is twofold:

1. Limit access to only what is needed for the job
2. Limit the attack surface to only what is needed for the job

The wise defensive coder understands that 1 and 2 are really the same rule written in different ways.

The whole point of limiting the attack surface is that we don't know what the attacker could possibly use it for, so give them as few attack vectors as possible to add to their weapon inventory.

All API surface is attack surface. IoT teddy bears and baby monitors are being used to launch DDoS attacks. Web browsing sessions are being used in clustered botnets -- stealing the CPU power of users of your app to crack passwords or encrypted data. You have no idea what attackers can or can't do with your stuff, so don't give them more stuff than you need to.

In case you don't get it yet, API surface area = weapons for bad guys. Don't leave them laying for attackers to pick up, even if you think you know it's safe.

When it comes to security, what you think you know and reality are usually two very different things. If you point a group of hackers at some seemingly innocuous app and give them the challenge, "figure out how to use this for evil", they will amaze, delight, and horrify you with their creativity.

Every software dev should probably read back issues of 2600 now and then just to develop a healthy insecurity about app security.

[omeid]

@ericelliott

Eric, the privileged methods that you speak of is a hack, it breaks jsdoc, jstags, doesn't play nice with flow type, and a fair bit of other tooling.

I fail to see how using a hack, regardless of the cost --and mind you, it is pretty high in this case-- is better than delegating that task to your linter?

The argument of security does not hold, as soon as you're executing untrusted code in the same context, all bets for security is off. Executing untrusted code in the same context and expecting closures to provide you meaningful secrecy or security is absurdly naïve.

And no. In OOP, the canonical way to "distinguish" between implementation details and public APIs is to encapsulate the implementation details so you can't get at them at all from the public API.

Canonical according to what or who? Most languages that pioneered and championed OO allows you to by pass the visibility mechanism in one way or another. So arguing that canonical OO requires strict encapsulation is false.

Your comment about security is absolutely true but also equally irrelevant; we are not talking about IPC APIs, we are talking about Object APIs of the code that runs in the same context, and as I said before, once you run untrusted code in the same context, all bets for security are off.

[ljharb]

"doesn't play nicely with tooling" is a secondary concern; inferior tooling shouldn't impact the way you write your code.

A linter lets you regulate your own code. You can't use a linter to prevent other people from accessing underscored properties in objects you hand out to them.

All code you didn't write is "untrusted".

[omeid]

@ljharb

"doesn't play nicely with tooling" is a secondary concern; inferior tooling shouldn't impact the way you write your code.

Sure, but throwing out tooling without a good reason is also a terrible decision.

On a side note, the argument that tooling shouldn't impact the way you write code is kinda ironic considering we are discussing the configuration for a tool that does exactly that, not only it impacts but explictly dictates how you should write code.

A linter lets you regulate your own code. You can't use a linter to prevent other people from accessing underscored properties in objects you hand out to them.

That is a good point. But people should read the docs, regardless of if they share the same QA infrastructure or not, and the docs should clarify what parts are considered unstable/private, and if people still decide to use unstable APIs, as @QuentinRoy said, it is their cat and their problem, changing the microwave and throwing away tooling to prevent people who choose to shoot themselves in the foot is a poor trade off.

All code you didn't write is "untrusted".

Which is the whole point, if you're running untrusted code, all bets for secrecy are off; but that is an orthogonal concern anyways, Encapsulation and Visibility -- and by extension the use of underscore as convention to denote privacy -- is not a security concern, it is for separation of concern and differentiating implementation from the interface.

[ljharb]

If tooling impacts your ability to write code, then that's the best possible reason to throw it out, immediately.

eslint does not impact your ability to write code (you can override it any time you like), it improves your ability to write code. There is nothing about eslint, or any eslint config, that forces you to write code a certain way; you can always override it.

"People should read the docs" is irrelevant - people should do lots of things. What people can do is what's important. Nobody should be going the wrong way on a one-way street, but because it's possible, you definitely need to be looking both ways before crossing it.

Again, running untrusted code does not erase "all bets" for secrecy and robustness - it only erases a large category of them. The only thing that makes "all bets off" is if untrusted code runs before your code, at which point, essentially nothing can be secured.

The interface contains every observable aspect of it. If _foo is observably present, it's part of the interface, no matter how much your documentation may try to insist otherwise.

[omeid]

@ljharb You have a very peculiar definition of impact, but I agree that linting is intend to improve code, which effectively impacts it.

Again, running untrusted code does not erase "all bets" for secrecy and robustness - it only erases a large category of them. The only thing that makes "all bets off" is if untrusted code runs before your code, at which point, essentially nothing can be secured.

That is a pretty bold and risky proposition, and unless the same guide forces one to freeze builtins, useArray.$method and Object.$method instead of someArray.$method, someObj.$method et al, and a whole heap of other concerns, then the argument that using closure would provide you secrecy is pretty short sighted.

Even then if you take all the necessary precautions and measures, running untrusted code in the same context that you have secrets is foolishly naïve. It is that simple.

But then again, that is all irrelevant. I must iterate that Encapsulation and Visibility -- and by extension the use of underscore as convention to denote privacy -- is not a security concern, it is for separation of concern and differentiating implementation from the interface.

The interface contains every observable aspect of it. If _foo is observably present, it's part of the interface, no matter how much your documentation may try to insist otherwise.

Interfaces are protocol and contracts and like any other, they come with requirement and stipulations, if one opts to ignore the warning and recommendation of the API then they're on their own; if one does dumb things they end up in stupid situations.

[QuentinRoy]

Guys, I am sorry but I think this is obvious for everyone that this is not going anywhere and is useful for neither the participants nor the readers.
I was quite happy to see @ericelliott providing some data. I wish we could go in a path a bit more scientific like this. I am a bit sad it was not really followed on.
Without data (and I think you guys have a lot of this), this discussion is about opinions and ideas. I think we clearly reached the end of it and are currently locked inside a bad while(true) argue().
I respect opinions (in particular from experienced people). But I trust data. Properly analyzed, that produces knowledge.

[JohanG2012]

hmm.. As I have understood it, this is basically a discussion about whether or not underscores should be allowed for method names. I kind of agree with the whole discussion about it not being private etc. I have however seen ALOT of React developers using underscore for states which are only UI states ex. the state "collapsed" in a menu component. This is something that I thought made sense and was doing myself until I started using this config and got a ton eslint errors. Up to this day, I still think this a good use of underscores and makes the code easier to reason about. But this is an aspect that not very many people seem to agree with or put a thought too over here.

Just wanted to add my 5 cents to the discussion.

[kmmbvnr]

• Google MDC uses underscore as a suffix.
• Facebook-relay uses underscore as a prefix.
• And Airbnb uses no underscore.

I experimented with all approaches.

• "No private" is not an option for me, b/c it makes hard to reason about the class, and I'd like a rule that every public method should be documented. It forces you to think about your interface. I don't care about real data privateness, but I never understand sentences like "intractable people problem", b/c undesired underscores access easily can be spotted on a code review unlike variables without any mark.
• Underscore as suffix works badly with my eyesight. I constantly miss them, and can't spot an error on a quick look over a codebase.
• So I choose to use underscore prefixes for private methods. And just consider event handlers are part of public class API (b/c they called outside), so no underscores for them.

[ljharb]

Once class instance fields reach stage 4, you won’t have to compromise on it.

[shelacek]

Once class instance fields reach stage 4, then I'll just find and replace my underscore prefixes ;-).

[ljharb]

@shelacek and then that will be a semver-major/breaking change for each file in question. As long as you understand that, then go for it.

[shelacek]

@ljharb Yeah, this is the core of this discussion. We have 2 groups of people here. For one, this will be change in the revision, because there is virtually no change in API. For the other, this will be a major change simply because it does not recognize this convention. Nevertheless, thank you for your opinion.

[beenotung]

Private class field is now supported by some runtime.
Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields

If you're stuck in the runtime that doesn't support private class field, and you want to prevent class consumer to access the private fields and methods, below are some workaround:

• hide private methods with local functions
• hide private fields with closure or local hashmap

Example for javascript runtime that doesn't support private field natively:

person.js:

let persons = new Map()

// "private method"
function getAge(self) {
  return persons.get(self).age
}

class Person {
  constructor(name, age) {
    // public field
    this.name = name
    // "private field"
    persons.set(this, { age }) // private field
  }

  // public method
  speak() {
    // calling "private method"
    console.log('inside:', this.name, getAge(this))
  }
}
module.exports = Person

consumer.js:

let Person = require('./person')
let alice = new Person('Alice', 18)
alice.speak()
console.log('outside:', alice.name, alice.age)

console log:

inside: Alice 18
outside: Alice undefined

[ljharb]

You'd want to use a WeakMap, not a Map, to avoid a memory leak, but yes, private class fields are a 100% safe and ideal approach for something you might otherwise stick an underscore on.