jsx-pascal-case: Allow leading underscore

[paulmelnikow]

I often find myself defining a component which wraps a component of the same name. In this case I usually import the wrapped component with an alias, like this:

import { Scene as _Scene } from '../scene/scene'

function Scene() {
  return <_Scene {...} />
}

This is triggering jsx-pascal-case.

I see there have been a few PRs to address edge cases around jsx-pascal-case to accommodate components called _ or SCREAMING_SNAKE. What do you think about allowing _PascalCase?

I'd be happen to make a PR.

[ljharb]

A leading underscore there seems like a poor choice; other than your familiarity, it doesn't seem like it conveys anything useful.

In this example, what is the difference between the Scene you're importing and the one you're exporting? Why do they have the same name?

[paulmelnikow]

The Scene I'm importing is generic, whereas the one I'm exporting is specific to the module it lives in.

Another way people do this is import Scene and then define / export MyScene or SomeKindOfScene. But, since I'd prefer to export Scene, it's cleaner to rename the import.

Since leading underscore are commonly used to designate things which are internal or private, as this alias for the imported component is, it seems like a pretty natural way to handle this.

It's similar to a convention I use with React.forwardRef:

function _Component(props, ref) {
  ...
}

export const Component = forwardRef(_Component)

However, this doesn't trigger jsx-pascal-case because _Component is not used in JSX.

[ljharb]

To be clear, renaming the import is definitely the proper solution. However, why not import { Scene as GenericScene }?

[paulmelnikow]

That's probably a better name. It still seems strange to me that this rejects leading underscores, especially given the edge case carved out by e7f7e89. Although, if you think it's what this rule should do, I get it.

Another option would be to enforce this strictly by default, and add options to opt into these edge cases without turning them on for everyone.

[ljharb]

Fair point - I agree with you that given that exception, this should be allowed (assuming that React itself allows it).

Regardless, you should still pick better names imo :-)

[itsjohncs]

I use leading underscores for component names sometimes, they have meaning in my codebase.

The ignore configuration option only allows strings, adding an option that takes a list of regular expressions would be a clear improvement.

As it stands I've just disabled the rule to avoid having to add exceptions manually.

[mwaddell]

Since JavaScript does not support "protected" or "private" access modifiers, it is common usage that anything prefixed with an underscore should be treated like "protected" and not accessed directly outside of a module or its unit tests. (Same with Python)

For example, lets say that I have a functional component that I want to break up into pieces to reduce the overall complexity of the main component:

export function Diagram(props) {
    return <>
        <_Header>My Floorplan</_Header>
        {props.rooms.map((r) => <_Room path={r.path}>{r.name}</_Room>)}
        <_Legend />
    </>;
}

export function _Header(props) { ... }

export function _Room(props) { ... }

export function _Legend() { ... }

Sure, I could rename those as DiagramHeader, DiagramRoom and DiagramLegend, but that doesn't convey to any other developers that these should be considered "protected" and shouldn't be used outside of the Diagram class and its unit tests. Renaming them would also make it less obvious to a user of this module what is the actual entry-point they should code against.

Yes, I could remove the export from these "protected" functions so they can't be used as entry-points, but then I lose the ability to unit test them directly, so that's not a good option either.

Like @itsjohncs, I've disabled this rule but would love to enable it again if it supported an allowLeadingUnderscore option (or if it allowed full regex expressions in the ignore option so I could allow these explicitly myself)

[ljharb]

@mwaddell common, but abysmally wrong and dangerous. Everything reachable is public, as is anything starting with "_". If you want private things on a class instance, use class private fields; if you want them anywhere else, use lexical scoping.

If they're exported, they are UTTERLY unprotected. If you want to prevent developers from using them, don't export them in the first place.

and yes, you should not BE testing private things directly. Test Diagram only, in a way that exercises those inner components.

[mwaddell]

I have to disagree about it being "abysmally wrong and dangerous" - it is a coding convention meant to overcome a limitation of the language (namely that there is no support for protected/internal/package/etc scoping) that most compiled languages provide.

I completely understand that there are no language-level protections enforcing that such code isn't called, it's purely a convention. However, it's a very convenient way to help organize large codebases into encapsulated modules and to provide guidance to other developers on how code is intended to be used. Finding import { _method } from "../otherModule"; is a very easy-to-spot "code smell" where import { method } from "../otherModule"; is not.

The example code I listed above is incredibly oversimplified, but there are legitimate reasons why you might want to have methods which are shared within a module/folder/etc - but which should not be used outside of that because it leads to inter-module coupling that makes your code very refactoring-hostile and hard to debug.

I also agree that, ideally, your code should all be clean enough that you can easily call every path in all private methods just by varying the inputs for the main method, but in practice that's not always trivial or requires mocking out an excessive number of other modules.

[ljharb]

The language no longer has that limitation - class private fields exist, and node’s “exports” field exists. There remains zero excuse to still be using underscores as such a convention.

[mwaddell]

Yes, the language absolutely still has that limitation. Although JavaScript does support the concept of private vs public, there are no other levels such as protected (accessible only to child classes), internal/package (accessible only to items within the same namespace/folder), friend (accessible only whitelisted namespaces/folders), etc.

For large, complex projects, there are legitimate reasons to have levels in between "public to the world" and "private only to this one file" -- these levels have more to do with enforcing good coding practices (e.g. code organization, encapsulation, reducing class coupling, etc) and less to do with trying to protect against malicious coders trying to hack your application.

I do agree that allowing leading underscores by default is not appropriate, but - just as jsx-pascal-case has an "ignores" list to allow users to customize its behavior, having this as an option for those teams who choose to use this convention - understanding that it is different from "private" - would be a useful addition to this rule.

However, if you are uncomfortable with even offering this as an option, then I would suggest that a way to address this (and other similar use cases) would be to add support for full regex patterns as an "ignorePatterns" option so that users can add more flexible whitelists than is supported currently by minimatch.

[ljharb]

"protected" is impossible in JS, and attempting to do it in any way is a category error for the language. JS does not have "access levels", it only has "reachable" and "unreachable". node's "exports" field provides "package" protection for any package inside node_modules, so this is not a justification.

Full regex patterns are always a terrible idea to support in an eslint rule, because they rapidly cause tons of CVEs to be filed (ReDOS, primarily). Globs would work, but I don't think this is a valuable addition.

[mwaddell]

"protected" is impossible in JS, and attempting to do it in any way is a category error for the language.

Someone should tell that to the TypeScript team, seeing as how TypeScript is basically just syntactic sugar around JS...

https://www.typescriptlang.org/docs/handbook/2/classes.html#protected

[ljharb]

It’s not, in fact; it’s not a superset of JS in any way. It’s a completely distinct language.

I also have told the typescript team this; ive been told they agree but don’t care because they’re giving their demographic what it thinks it wants.

[mwaddell]

My only remaining question then, is why is this even an open issue? The only possible reason for allowing leading underscores (or any other non-standard naming convention) is to convey information to other developers based on those names. You have made it clear that this is not something that eslint will ever support regardless of what developers "think they want," so why wasn't this issue closed over a year ago as "wontfix"?

[ljharb]

Per #2603 (comment), it will be accepted. It's just that "to convey privacy" is a terrible reason to do it, and I'll want to make sure the documentation does its best to shatter anyone under that harmful delusion.