airbnb / javascript

JavaScript Style Guide
MIT License
145.51k stars 26.54k forks source link

Question about new `new-cap` rule #1106

Open dandean opened 8 years ago

dandean commented 8 years ago

I don't want to bikeshed on this, but I do have some concerns about this rule change I'd like to bring up.

My biggest concern is that it will lead to more people overriding the new-cap rule than in the past because this new rule is so limited in scope. This would be the opposite of the intended outcome of #1089.

The result is that eslint will tell people to use new for new Immutable.Record() AND to NOT use new for Immutable.Map(). The only way to make this consistent is to override the rule in your local environment, and this will be incredibly common.

My suggestion is to either:

If this interests you I'm happy to submit a patch for either choice.

ljharb commented 8 years ago

I think using the pattern for Immutable.js (as long as it requires you omit the new in those cases) is the ideal approach.

dandean commented 8 years ago

👍

A quick test shows this working:

    "new-cap": [
      "error", {
        "capIsNewExceptionPattern": "^Immutable.\\w"
      }
    ],

BUT it does not require the developer to omit new. I don't see any obvious way to enforce the omission of new for Immutable.* functions. I'll keep digging.

ljharb commented 8 years ago

What about also adding newIsCapExceptionPattern that's the same?

dandean commented 8 years ago

I tried that as well, but it leads to allowing constructs like:

new Immutable.foo();

In other words, that configuration allows the name of the constructor used with new to be lower case.

allows any lowercase-started function names that match the specified regex pattern to be called with the new operator.