feat(eslint-plugin): [interface-name-prefix, class-name-casing] Add allowUnderscorePrefix option to support private declarations

[octogonz]

The _ prefix is sometimes used to designate a private API, in which case a private interface might be
named _IAnimal instead of IAnimal. Update "interface-name-prefix" to recognize both forms.

[typescript-eslint]

Thanks for the PR, @octogonz!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint

[bradzacher]

Two questions:

• why do you want to use the I prefix for interfaces?
• why do you have "private" types that need to be explicitly marked as private? If it's a private type with no use outside of the internals of the package, then surely it just means nobody will use the type? What do you gain from marking it as private with an underscore?

[octogonz]

Oh man, I was hoping no one would ask that!

Like most coding style opinions, my secret motivation is to avoid changing thousands of files that do it a certain way. Some are commercially supported API contracts. So I'll be transparent about that heheh.

But there's also legitimate justification for I and _ prefixes. I do find it valuable. The case is a little convoluted, so we'll go in steps:

1. For starters, let's narrow this linting discussion to very large code bases. In this world I spend much of my time reviewing PRs and debugging source files that (1) I am seeing for the first time and have no idea how they work, and (2) are made by developers of varying skill levels and motivations (and also status of "still employed here and able to explain their code" or not). In this situation, slight improvements in readability really do matter. Whereas for smaller projects/teams, conciseness and flexibility may be more important than readability-by-strangers. (If so, stop here heheh.)

2. I will also point out that TypeScript's "type algebra" is very flexible. It was designed to provide accurate IntelliSense for even the nastiest legacy JavaScript patterns. It also enables interesting higher order programming. This can be contrasted with early C# or Java, whose type systems stick to a small set of constructs that I will call "stereotypes": functions, classes, interfaces, enums, properties, events, etc. TypeScript provides these stereotypes too, but as special cases of type algebra. For example, these are completely equivalent:

   // type algebra
   declare const Widget: { new(): Widget };
   declare type Widget = { render: { (): void } };
   
   // stereotype
   declare class Widget {
     public render(): void;
   }

   Type algebra is more powerful and flexible and expressive. But in a large code base with people of varying skill levels, I've come to the view that stereotypes are (1) more easy to read and talk about, and (2) fully capable of representing 99% of what we need to implement. In some cases this restriction produces more verbose code, but if we value readability over writability, it doesn't matter to much. In particular, very readable code is less likely to get thrown away and redone from scratch, which perhaps justifies preferring stereotypes and paying a tax of writing more verbose declarations.

3. Of these stereotypes, the interface is particularly significant. It often describes an abstract contract that someone has to implement. (I say "often" because TypeScript's interface has a dual purpose to describe object literals, but let's ignore that for now.) Thus it is helpful to use the prefix I to call out IWidget as special among the soup of other PascalCase names that are mostly concrete things. If there is a class that serves as the standard implementation, this prefix conveniently lets us to say Widget implements IWidget. By contrast, in Java we find ourselves inventing somewhat artificial whole word prefixes, e.g. Widget implements AbstractWidget or CommonWidget implements Widget or maybe Widget implements VisualRenderable.

4. The underscore prefix was commonly used in JavaScript to designate a private member. Consumers of a library learned a simple rule: "If you access a name that starts with _, you're doing something that is not unsupported. Careful!" This convention is useful in TypeScript, too. If the TypeScript library is consumed by plain JavaScript caller, the type annotations won't be available, so the underscore helps. For 100% TypeScript, the underscore prefix is also useful for package exports, as well as public class members, in the case where two closely related modules need to access each other's internals. (The same idea as C# internal or InternalsVisibleToAttribute or C++ friend.)

To tie it all together, suppose the Widget base class comes from the package ui-controls, and the package ui-renderer needs special access to some Widget state. The ui-controls package might export an internal interface _IWidgetDeviceHandle that is shared by these two libraries, but which should not be used by third party consumers.

[bradzacher]

I personally hate the IFoo naming convention, and I'm vehemently against it (you can do some searching issues and find my arguments..)

I can understand it if you don't use a rule like our consistent-type-definitions to enforce all your object types are one way or the other.

I still don't hugely understand why it matters if someone consumes an internal type. It's a compile time thing with no implementation, so it shouldn't matter. Unlike a class or function which might be exposing some internal implementation details, a type is just a shape. But I can kind of understand the b2b setting of commercially consumed libraries and trying to help consumers understand what they should/shouldn't access.

If your packages are in a monorepo, you could actually achieve this in a better way. If you add /** @internal */ to your types, it will cause typescript to skip it from the generated type definitions.
I believe that if you use tsconfig's project configurations to link your projects in a monorepo, then it will not use the generated type definitions, so it bypasses this. But your external consumers will not see the types.

The typescript package uses this copiously to hide a lot of things from the types without impacting their internal devx.

[bradzacher]

I won't block this.
It ultimately won't affect our recommended default of never, nor will it harm anyone using the rule right now.

I would ask that you instead hide this new functionality behind an option, so that people have to opt-in to it. Otherwise it's a breaking change because the you're technically making the rule "less strict"

I.e. add a third string:

• never = don't use IFoo or _IFoo
• always = use IFoo
• always-allow-underscore = use IFoo or _IFoo

[octogonz]

I can understand it if you don't use a rule like our consistent-type-definitions to enforce all your object types are one way or the other.

Why should that matter? The consistent-type-definitions rule allows us to prevent people from using type algebra when they should be using the interface stereotype instead.

I still don't hugely understand why it matters if someone consumes an internal type. It's a compile time thing with no implementation, so it shouldn't matter. Unlike a class or function which might be exposing some internal implementation details, a type is just a shape.

It would be weird to use _ for functions, classes, enums, etc but NOT use it for interfaces.

If your packages are in a monorepo, you could actually achieve this in a better way. If you add /** @internal */ to your types, it will cause typescript to skip it from the generated type definitions.

We use that, too. (Or rather API Extractor's .d.ts rollups that support @public, @beta, @alpha, and @internal depending on the release target.) But you still want _ for consumers that don't use TypeScript, and it would be weird to only use the prefix only for certain kinds of constructs.

The change that I proposed doesn't /require/ anyone to use the _; it merely allows it if people want to use it.

I would ask that you instead hide this new functionality behind an option, so that people have to opt-in to it. Otherwise it's a breaking change because the you're technically making the rule "less strict"

I.e. add a third string:

• never = don't use IFoo or _IFoo
• always = use IFoo
• always-allow-underscore = use IFoo or _IFoo

The typescript-eslint package is still fairly rough around the edges. Do you consider this to be a significant breaking change? I mean if we are being pedantic it should probably be a separate setting ignoreUnderscore so we don't alter the never semantics either... but I wonder if any actual user would realistically be impacted by this. Seems unlikely.

[bradzacher]

Why should that matter? The consistent-type-definitions rule allows us to prevent people from using type algebra when they should be using the interface stereotype instead.

Because if you're consistently enforcing interfaces, then there isn't any need to name them differently. But that's a whole other thread (feel free to search, I've ranted ad nauseam about this).

Do you consider this to be a significant breaking change? I mean if we are being pedantic it should probably be a separate setting ignoreUnderscore so we don't alter the never semantics either

I don't think it's a breaking change for never users, because they are not going to be naming interfaces matching either regex, so if anything this will help catch cases that were missed before for them.

For always users, I think the looseness might be something undesirable for a non-trivial subset.
TBH I've never seen anyone use this pattern before, so I think there would be people that don't want to allow it in their codebase.

[octogonz]

The debate about I being good/bad seems slightly off topic here. The default is already never, and that makes sense to me, as that practice is more prevalent. But there are plenty of users who do want to use I, though, and my immediate need is to enable ESLint to work for them.

(BTW I had trouble finding the comments you referenced, since there's a bunch of flame wars about this topic. If you're interested to chat about it offline, I'm reachable on Gitter. It might be worth discussing, as I am involved with some influential projects, so if you can convince me to abandon I then you'll get more converts heheh.)

For always users, I think the looseness might be something undesirable for a non-trivial subset.
TBH I've never seen anyone use this pattern before, so I think there would be people that don't want to allow it in their codebase.

HBO and Microsoft both use the _ prefix extensively to indicate private members.

Your point seems reasonable, though. I'll add always-allow-underscore as a third option.

[bradzacher]

HBO and Microsoft both use the _ prefix extensively to indicate private members.

as does facebook - for private class members. They in fact have a transpilation step which converts underscore prefixed members to be private outside the class definition:

class Foo { _priv = 1 }
// transpiles to
var Foo = function Foo() { this.$Foo_priv = 1; };

And to some degree for private exports that need to be exposed, but shouldn't be used, though in that case they usually use "goofy" naming to further show they're not to be used :P
https://github.com/facebook/react/blob/80bff5397bf854750dbe7c286f61654ea58938c5/src/umd/ReactUMDEntry.js#L20-L23

But I was referring to types specifically - I haven't seen underscored private types before in either flow or ts codebases.

[octogonz]

But I was referring to types specifically - I haven't seen underscored private types before in either flow or ts codebases.

Here's an example: sp-application-base

All those entries that say /* Excluded from this release type: _IExtensionContextParameters */ are classes/interfaces whose source code uses an underscore prefix. The declarations get trimmed out of the published .d.ts rollup, but the classes are still exported from the JavaScript bundle (so that they can be imported by the "friend" packages), and are thus visible to customers. The interface types are gone at runtime as you said, but in that code base it would be weird for people to have to remember whether to use _ or not according to whether the construct has a runtime representation or not. Also within the original source code, the name _IExtensionContextParameters helpfully reminds everyone that the interface is internal, so people don't use it inappropriately.

I totally get that such requirements may be statistically rare. It's possible these concerns are irrelevant to 99% of the humans that write TypeScript code, most of which are small causal projects. But that "1%" minority is still a lot of developers and a lot of money, just closed-source and not super visible on the internet.

[octogonz]

@bradzacher I am hitting this same issue with class-name-casing as well, for a package that exports an internal class name like _Widget.

Maybe we should try to make them consistent, e.g. add an option allowUnderscorePrefix to both rules?

[bradzacher]

based on the current config style of this rule, I don't think it's the correct thing to add an extra object option to this.
We're trying to avoid having multiple config options for rules i.e. ["error", "always", {"allowUnderscorePrefix": true}], as people can get pretty confused about the second element in the options array.

In this case we can do it easily with just another string in the enum.
Ideally we'd change the config to be an object - as that's what we're standardising on now.
But that's a breaking change, and we won't be able to get this in before the 2.0.0 release, so then it'd have to wait for the 3.0.0 release... An extra string in the enum is the easiest way to get it in asap.

---

class-name-casing has no options, so there it makes sense to add its first one, which would be an optional options object with the single property.

[octogonz]

We're trying to avoid having multiple config options for rules i.e. ["error", "always", {"allowUnderscorePrefix": true}], as people can get pretty confused about the second element in the options array.

Does ESLint support polymorphic options? In other words, this:

["error", "always"]

...as a shorthand for this:

[
  "error", 
  {
    "prefixRequired": "always"
  }
]

...which can be expanded to this:

[
  "error", 
  {
    "prefixRequired": "always",
    "allowUnderscorePrefix": true
  }
]

That might enable new options to be introduced without breaking compatibility with the old schema.

class-name-casing has no options, so there it makes sense to add its first one, which would be an optional options object with the single property.

@bradzacher What do you want the option to be? I can implement it today and just add it to this same PR, since it's the same scope of work.

[bradzacher]

It does support it. You'll have to setup the json schema to support it. You can use the oneOf schema to make it work.

That's a definite path to go, I didn't suggest it because it's more work: change the schema, change the options type def, add support for handling both options forms. You'd also need to duplicate tests to make sure both options forms work for existing options. If you're happy to do the extra work, then I'd love to see it, as it's the best of both worlds.

Would suggest the following schema:

type Options = [
  | 'never'
  | 'always'
  | {
    allowPrefix: false;
  }
  | {
    allowPrefix: true;
    allowUnderscorePrefix: boolean;
  },
];

/*
'never' === { allowPrefix: false }
'always' === { allowPrefix: true, allowUnderscorePrefix: false }
*/

// keep the default backwards-compatible
defaultConfig === { allowPrefix: false }

---

For class-name-casing, I would suggest the following schema:

type Options = [
  {
    allowUnderscorePrefix: boolean;
  },
];

// keep the default backwards-compatible
defaultConfig === { allowUnderscorePrefix: false }

[octogonz]

I don't mind the extra work. This design seems fine.

BTW it wasn't immediately obvious to me that class-name-casing applies to interfaces, too, and why it does not apply to type aliases. I wonder if longer term it should be generalized into a "type name casing" rule.

[bradzacher]

definitely - someone has already brought that up #671!

[octogonz]

@bradzacher I'm realizing that allowPrefix: true; is somewhat confusing, since we don't merely "allow" it; we are /requiring/ it. Also, the name doesn't clearly indicate that we're talking about the letter I prefix.

Something like prefixWithI: boolean or iPrefix: 'always' | 'never' would be more clear. Lemme know what you prefer.

[octogonz]

I went with this:

type Options = [
  | 'never'
  | 'always'
  | {
    prefixWithI?: 'never';
  }
  | {
    prefixWithI: 'always';
    allowUnderscorePrefix?: boolean;
  },
];

[octogonz]

I pushed an update for interface-name-prefix. I will fix class-name-casing in a bit. Both changes should go in this same PR, since otherwise the rules interfere with each other.

[octogonz]

This is ready to go, and all the checks are green. Please take a look when you get a chance. Thanks!

stale

[bradzacher]

it all LGTM!
Thanks for doing this!

[octogonz]

Ping... Is there anything holding up this PR from getting merged? Thanks!

[octogonz]

Thanks! 😁