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

Usage of underscore for internal methods in React components #509

Closed
javiercr opened this issue Nov 26, 2016 · 8 comments · Fixed by #510
Closed

Usage of underscore for internal methods in React components #509

javiercr opened this issue Nov 26, 2016 · 8 comments · Fixed by #510

Comments

@javiercr
Copy link
Contributor

javiercr commented Nov 26, 2016

It seems many people are abandoning the use of prefixing "internal" methods of a React component with an underscore (eg: _handleOnPress). The Airbnb React's style guide explicitly discourages this:

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

Why? Underscore prefixes are sometimes used as a convention in other languages to denote privacy. But, unlike those languages, there is no native support for privacy in JavaScript, everything is public. Regardless of your intentions, adding underscore prefixes to your properties does not actually make them private, and any property (underscore-prefixed or not) should be treated as being public. See issues #1024, and #490 for a more in-depth discussion.

Right now in Ignite the use of underscore to prefix "private" or "internal" methods is not consistent.

For example, DrawerContent does not use underscore:

handlePressComponents = () => {
    this.toggleDrawer()
    NavigationActions.componentExamples()
  }

While ListviewExample does use underscore prefixing:

  _renderRow (rowData) {
    return (
      <View style={styles.row}>
        <Text style={styles.boldLabel}>{rowData.title}</Text>
        <Text style={styles.label}>{rowData.description}</Text>
      </View>
    )
  }

Personally I'd prefer removing underscore, as the Airbnb style guide suggests. If you guys are ok with this I could submit a PR.

To avoid future problems for this, maybe Ignite should subscribe or recommend some style guide.

@javiercr javiercr changed the title Use of underscore for internal methods in React components Usage of underscore for internal methods in React components Nov 26, 2016
@kevinvangelder
Copy link
Member

You're correct. We should have an opinion on this and we should be consistent. As for which direction we should go, I'm sure there will be a variety of opinions, and my GAFO meter is low enough that I could be happy either way.

@GantMan
Copy link
Member

GantMan commented Nov 26, 2016

Hey @javiercr - let's make the decision!

So you and Steve Kellock are both for removing underscore for private methods. I have to say, I don't really understand or agree with AirBnB's reasoning, so maybe you can convince me? I get that it's not enforced by the underlying language... but then again.. neither is camelCase.... We just do that as a convention, and if someone were to violate camelCase, then "The language doesn't care" isn't a good argument, right? So why is it a good argument here?

@javiercr
Copy link
Contributor Author

Thank you guys for the quick feedback.

Just to be clear, I'd remove underscore prefixes just because that's what I see most people are doing in recent JS / React projects – I can't find any strong argument to do so neither, it's just a matter of consistency :)

@skellock
Copy link
Contributor

There's ways to design your code so you don't expose private functions. The reveal pattern and factory functions do this.

Where we do use it, in React components, every method you hang on a React component is effectively invisible anyway. We don't need to distinguish between private & public, we use props.

Typescript (because it came from OO minds) has a private keyword.

Flow has munge_underscores which, if I'm reading right, obfuscates??? (that's horrible if it does).

I'd be happy to see these go.

@GantMan
Copy link
Member

GantMan commented Nov 27, 2016

Correct, we don't need to distinguish between public and private. It's a friendly readability thing. That's why this convention still lives in Python today.

every method you hang on a React component is effectively invisible anyway

Every so often you have a function that's not part of the component lifecycle, and you want to call it. this.refs.scrolly.doSomethingSpecial etc. Encapsulation can be broken, so I guess the utility of _ is to identify safe breaking of encapsulation.

Where I think Steve has a problem with this, is that he'd rather use a closure to actually enforce privacy. Because he's 1337 like that. But my functional programming brain is rusting by the minute.

I know JSDoc supports documenting certain methods as private, and maybe that's a better route?

TL:DR;

I agree, we need to make the code uniform. I love the idea of a PR, but I just want to make sure we chose the best strategy and get behind it for a bit. "1 hour of planning saves 3 in practice"

Currently I'm leaning towards removing the underscores because logically they aren't that important. But my code kata, and discipline wants visual/logical indicators to identify intention.

@skellock
Copy link
Contributor

That's not what I'm saying. Haha. =)

I wouldn't escalate this to "i think functionally and you don't".

If you're using closures or using refs, you're buring state. And that's totally ok if that's what you need & want.

The point I'm trying to make is actually a lot less important & meaningful than that...

The _ identifier was used for the last 10 years in Javascript as "don't call this from the outside". Since props is the vehicle for inter-component communication, we don't need to distinguish between private or public, because it all should be private.

I've seen code with & without _ from people I admire.

My preference is without.

Just like my preferences is 4 space indentation, decorators, hockey over football, and coke over pepsi.

❤️

@GantMan
Copy link
Member

GantMan commented Nov 27, 2016

"4 space indentation" - dumpster fire! 2 spaces for life, son...
"decorators" - yes my house could use them. Green goes with turquoise right?
"hockey over football" - We can't keep ice here in the south, so might as well be Quidditch
"coke over pepsi" - awwww yeahhhhh!
"we don't need no damn _" - yeah, ok.

Components don't need underscores, sure. We do have some objects, too. We're not 100% components, but my degree of care for the utility _ is low. Seems lots of smart people seem to care, so let the fad be!

@javiercr - your PR would be appreciated 👍
Also - did you get contacted about your free ignite shirt?

@javiercr
Copy link
Contributor Author

PR created. @GantMan yup, the t-shirt is on its way, thank you guys! :)

GantMan pushed a commit that referenced this issue Nov 27, 2016
…#510)

* Remove underscore prefixing for “private” methods in React components – fixes #509

* Remove underscore prefixing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants