Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Underscore prefixes for "private" properties #490

Closed
taion opened this issue Aug 27, 2015 · 36 comments
Closed

Underscore prefixes for "private" properties #490

taion opened this issue Aug 27, 2015 · 36 comments

Comments

@taion
Copy link
Contributor

taion commented Aug 27, 2015

From the main section:

Use a leading underscore _ when naming private properties.

From the React section:

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

These two seem a bit inconsistent.

@goatslacker
Copy link
Collaborator

A bit. The thing about React components is that all methods should be private so we just save ourselves the typing and don't insert the leading underscore.

@taion
Copy link
Contributor Author

taion commented Aug 28, 2015

What do you mean? If I have e.g. a ref to an element, I can call methods on it just fine. I've seen this used to implement e.g. getValue methods on things like complex, uncontrolled input components for forms.

If you consider this an anti-pattern, it could be useful to add a clarifying comment to say to not do this. There are definitely problems with this pattern - it's not container-safe, for one.

@afilp
Copy link

afilp commented Oct 25, 2015

I am also interested in an explanation to that. Not all methods are "internal" if we consider that we may call a component's method from the parent.

Example:

parent.jsx

    _handleOpenChildDialog() {
        this.refs.childComponent.showDialog();
    }

childComponent.jsx

    showDialog() {
        this.refs.myDialog.show();
    }

So, what is our general take on this? Do we still write all methods without any underscore?

@taion
Copy link
Contributor Author

taion commented Oct 25, 2015

👍 Facebook, for example, use this pattern extensively: https://github.com/facebook/relay/blob/v0.4.0/src/container/RelayRootContainer.js

@hbrls
Copy link

hbrls commented Oct 26, 2015

@taion what about v0.14

@taion
Copy link
Contributor Author

taion commented Oct 26, 2015

What about it?

@ljharb
Copy link
Collaborator

ljharb commented Oct 26, 2015

Personally, I think this pattern is horrifically bad. Things that are not private are not private - sticking an underscore in front of them doesn't make them private, it just lulls you into the false belief that you can make a breaking change without bumping a major version.

It's not private if it's accessible in the runtime. If it's private - ie, via closure or WeakMap - no convention or unit tests are needed. If it's not, it's public, and you must test it, and whether you document it or not, it is public and breaking it counts as a breaking change. At that point, using a leading underscore is just fluff, so why would you need a convention for it?

@taion
Copy link
Contributor Author

taion commented Oct 26, 2015

@ljharb

That's totally fine - I personally don't mind writing my code either way (as long as I can point to a style guide and say that I'm following it).

The issue is that this style guide appears to be inconsistent between https://github.com/airbnb/javascript#22.4 and https://github.com/airbnb/javascript/tree/master/react#methods. I don't see how it's possible to follow both.

@ljharb
Copy link
Collaborator

ljharb commented Oct 26, 2015

React is a special subset - you could follow both by using _-prefixes in non-React code. I don't see a logical disconnect there.

@taion
Copy link
Contributor Author

taion commented Oct 26, 2015

Okay. If that's intentional, I can live with it.

@ljharb
Copy link
Collaborator

ljharb commented Apr 12, 2016

JS doesn't have privacy. Underscore-prefixed things are just as public as anything else.

@goatslacker
Copy link
Collaborator

We should probably just remove this section from the main style guide or rephrase it. Same with the React style guide. This whole attachment to "private" is what makes the rules seem confusing.

@taion
Copy link
Contributor Author

taion commented Apr 12, 2016

Either way would be fine – it's the inconsistency that's galling.

Though I'd note again that FB do in fact use leading underscores, real privacy or no 😛

ljharb added a commit to ljharb/javascript that referenced this issue Apr 12, 2016
ljharb added a commit to ljharb/javascript that referenced this issue Apr 12, 2016
ljharb added a commit that referenced this issue Apr 13, 2016
ljharb added a commit to ljharb/javascript that referenced this issue Apr 14, 2016
@danactive
Copy link

I'm late to this discussion, but if I drop the underscore, how will my colleagues know to avoid private methods that will change at any point. The absence of underscore indicates any change will be documented with a semver version bump.

PS: I export my private methods so they may be unit tested.

@ljharb
Copy link
Collaborator

ljharb commented Apr 26, 2016

If it's private, it should not be directly tested, nor accessible - only the public API should be tested. If it's exposed, for any reason, it's simply not private anymore, and changing it in a breaking way is simply a semver-major change.

@nicbou
Copy link

nicbou commented May 4, 2016

In principle, yes, but underscores are a widely understood indicator that a variable is "private". You don't walk through a door with a sock on the handle, even if it's unlocked.

Since there are no private properties in ES6 classes, how can I indicate "private" methods and properties? This is especially problematic with getters/setters in ES6, since it needs to be stored in a separate variable.

// Simplified example
export class User {
  get address() { return this._address; }
  set address(address) { this._address = address; }
}

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2016

@nicbou "widely understood indicator" isn't the same thing as "language definition". The answer to your question is that there are no private methods and properties, so you can't indicate them - you can't actually have them. This is a limitation slash feature of JS, and the proper solution is usually to architect around it rather than to work around it. (In that example, I'd use this.address and this.getAddress() and this.setAddress() - our style guide explicitly discourages using ES5 getters and setters)

@taion
Copy link
Contributor Author

taion commented May 4, 2016

@ljharb

This might just be a limitation in understanding. Suppose I have this.setAddress() and this.getAddress(), and this.setAddress() performs additional validation (or something).

I think the concern is that if I store the address field on this.address, it's relatively easy for consumers of this object to accidentally do this.address = foo rather than this.setAddress(foo).

While I can e.g. use a WeakMap to get an actual private field, maybe I don't need that, and just want a code-completion-friendly way to suggest to users that they should use this.setAddress() instead of this.address.

How do you deal with such cases? Doc comments are fine, but they have the downside that you can't see them unless you actually look at the code, while property names are visible given just an object.

@taion
Copy link
Contributor Author

taion commented May 4, 2016

More briefly, taking your example, how do you indicate that users should probably do obj.setAddress(foo) rather than obj.address = foo?

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2016

@taion Given that a WeakMap-per-field is the only way to get true per-instance privacy in JS, then I'd think just not defining "this.address" should work fine with IDE code completion - if not, they should be looking at the docs or the "class" implementation to see what options are available. setAddress is a function property on the object just like address would be.

@taion
Copy link
Contributor Author

taion commented May 4, 2016

I'm saying something a little different, I think – having this.setAddress() and this._address communicates that this.setAddress() is the preferred API, and that it would be unusual to set this._address directly, versus the WeakMap approach that gives actual privacy but is just more code.

The goal isn't to prevent determined users from accessing e.g. this._address; more to suggest that they prefer this.setAddress().

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2016

@taion if your goal is helpful suggestions, then name it this.private_address_do_not_use, or SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED. Naming it _address is just obscure.

@taion
Copy link
Contributor Author

taion commented May 4, 2016

I don't think it's especially obscure, though – it's a common convention, not just in e.g. Facebook code but also from earlier versions of this guide: https://github.com/airbnb/javascript/tree/eslint-config-airbnb-v1.0.0#22.4.

@nicbou
Copy link

nicbou commented May 4, 2016

@ljharb, I agree that this is not a language definition, but I disagree that we should architect around it when it's clearly a language limitation. Calling a private method SECRET_METHOD_DO_NOT_USE_OR_YOU_WILL_BE_FIRED does not make the variable any more private, nor does it prevent people from using them.

In this scenario, there is no perfect solution, only different hacks for JavaScript's limitations, so arbitrarily preventing one commonly used workaround seems odd to me.

As for underscore-prefixed variables being obscure, I completely disagree. If it were, you wouldn't set a rule for it, Mozilla wouldn't mention it, and we would not be having this discussion.

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2016

I think it's problematic because it's a common convention - because many people see it and think "private", and it is anything but. Accessible === public === changes to it affect semver. If you are personally confident that you, and all future contributors, will respect this responsibility on a given project, then you are of course most welcome to disable linter rules and/or ignore sections of the guide - but I think it's important that the default, and the general advice, errs on the side of safety and not false promises of privacy.

@jquense
Copy link

jquense commented May 4, 2016

👍 on underscores, while stuff may not actually be private that seems irrelevant, the point of privacy in programming is isolation, encapsulation for humans. Insofar as the convention is understood by humans then who cares whether its "really" private or not. The value of the underscore is in signally to users of your code what is and is not the public API. In my experience despite the potential for folks not following the convention, almost no one uses an underscored method or property thinking thats its something that is part of the supported public api of the package

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2016

@jquense node core has, and npm broke them accidentally. This is a HUGE problem in practice.

@jquense
Copy link

jquense commented May 4, 2016

This is a HUGE problem in practice.

well agree to disagree then on that. In practice it's not been a huge problem for anything i've been involved in. Signally is helpful, and imo better than the alternative of accidentally signally that everything is part of your public api, when you support in browsers that don't have a WeakMap, or the Object model doesn't easily allow closures for instance properties.

@nicbou
Copy link

nicbou commented May 6, 2016

@ljharb, that's great, but since there is no readily accessible solution to have private methods/properties in ES6 classes, what would you suggest? Without a usable alternative to underscore-prefixed classes, there really isn't a reason to dismiss them.

It seems to me that this rule takes the standard from "best practices" to "high-minded ideals". So far, all of the proposed changes have been about improving coding practices, rather than forcing awkward workarounds for language limitations.

In addition, I forgot to mention JSDocs, which has a @Private attribute as an extra layer of don't-do-this-you-dunce for whoever might want to access private attributes.

As a compromise, couldn't we raise an error for accessing _variables from another class?

@ljharb
Copy link
Collaborator

ljharb commented May 6, 2016

My suggestion continues to be, design things so you don't need privacy. Comments, JSDoc or otherwise, aren't enforcement any more than an underscore.

In JS, it is a bad coding practice to design something that runs into language limitations - in other words, you need to work within the constraints of the language.

This rule will not be changing. You're welcome to fork the guide, and/or overwrite linter rules, to fit what makes sense for your project.

@nicbou
Copy link

nicbou commented May 6, 2016

Yes, and I am asking "how?", because there clearly isn't a way to do so with ES6 classes.

@ljharb
Copy link
Collaborator

ljharb commented May 6, 2016

This isn't the proper venue for it, but if you ping me in the gitter channel, I'll be happy to show you how it's easily possible for your example.

@nicbou
Copy link

nicbou commented May 6, 2016

Sure thing! Let's talk about it Monday. The weekend is too short for arguments about code ;)

@gaurav21r
Copy link

@ljharb Your points are very valid and I guess the Javascript Ecosystem has a history of "working around the language" read CoffeeScript, TypeScript, React & JSX etc.

But you must agree the leading (and trailing) underscore is pretty much prevalent in the Industry.

And there are some more heavyweights too apart from the above.

Your point about working with the language is well taken. I personally feel though that this style guide could be better off taking a solid consistent stand throughout.

Eg with https://github.com/airbnb/javascript#accessors--no-getters-setters It seems we are encouraging people to go "around" the language and use custom setters like setProperty which is code convention based and not language based (getters & setters).

I may be wrong but this seems to be the same issue as the leading underscore/

@ljharb
Copy link
Collaborator

ljharb commented Aug 29, 2016

@gaurav21r I believe Polymer is from Google, so your three examples are really just two. Also, someone being a "heavyweight" is a poor reason to make any choice.

This style guide does take a solid and consistent stand: do not use ES5 getters/setters; and do not use underscores at all (trailing or leading), nor anything else that attempts to convey soft "privacy". The reasons are hopefully communicated well. If they can be improved, PRs are most welcome!

Just because a language provides functionality does not mean it is the best or most appropriate choice to achieve a goal, nor that using an alternative is "going around the language". You can do lots of things with eval or with, but that doesn't mean it's ever a good idea.

jaylaw81 pushed a commit to appirio-digital/ads-best-practices that referenced this issue Sep 19, 2017
sensiblegame added a commit to sensiblegame/React-BNB that referenced this issue Oct 23, 2017
andrius1 referenced this issue in devbridge/Front-End-Toolkit Jan 2, 2018
* Updated Scss file structure
* Added nvmrc file. Required node version is added, easier to use with nvm preinstalled.
* Scss-lint task is added and now running on the fly
* Resolved scss-lint issues in scss files, global scss-lint config edited
* Support added for nesting selectors with parents (resolved issue with bem selectors, when writting scss code)
* Added gitattributes file
* Automated image optimization added
* Added functionality, which checks if all dependencies are up to date. If not - gulp automatically installs them.
* Added npmrc file (dependencies are automatically saved, even without defining flags, when installing and current version is defined in package.json)
* eslintrc file is added
* starter-template folder is removed
* sass errors are not breaking gulp tasks
* WebPack is running from gulp
* Added gulp task cacheing for better task performance (faster repetitive tasks)
* Added ability to make scss-lint errors silent
@MatthewCochrane
Copy link

MatthewCochrane commented Jan 5, 2021

I didn't really understand all this fuss about truly private variables until I connected it with the concept of Hyrum's Law.

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

This is intuitive to me and makes clear sense. With that in mind and the scale of Airbnb it makes sense why this rule exists in their style guide and how 'fake private' is really just plain old 'public'.

passionSeven added a commit to passionSeven/javascript that referenced this issue Jan 27, 2023
Binary-Ninja-007 added a commit to Binary-Ninja-007/JavaScript_Style_Guide that referenced this issue Aug 13, 2023
harry908nilson pushed a commit to marcolane/Javascriipt that referenced this issue Sep 1, 2023
noakosar515 added a commit to noakosar515/JavaScript that referenced this issue Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants