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

Reconsider capIsNew: false for new-cap #2200

Open
kevinoid opened this issue Mar 22, 2020 · 2 comments
Open

Reconsider capIsNew: false for new-cap #2200

kevinoid opened this issue Mar 22, 2020 · 2 comments
Labels

Comments

@kevinoid
Copy link

With eslint-config-airbnb-base@8.0.0 the code

class Example {}
module.exports = Example();

would result in the error:

example.js:2:18: A function with a name starting with an uppercase letter should only be used as a constructor. [Error/new-cap]

As a result of #1090, which set capIsNew: false for new-cap, this is no longer the case.

#1089 (comment) which motivated the change only mentions adding "capIsNewExceptions": ["Immutable.Map", "Immutable.Set", "Immutable.List"]. Would it make sense to revert capIsNew: false (and perhaps adopt "capIsNewExceptionPattern": "^Immutable.\\w" as suggested in #1106) or was there another rationale for setting capIsNew: false?

Thanks,
Kevin

kevinoid added a commit to kevinoid/eslint-config-kevinoid that referenced this issue Mar 22, 2020
To match the behavior of eslint-config-airbnb-base@8.0.0 and before,
where functions starting with a capital letter must be called with the
new operator.

The motivation for setting `capIsNew: false` is not clear to me and the
value of catching incorrect constructor calls is non-zero.
See: airbnb/javascript#2200

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@ljharb
Copy link
Collaborator

ljharb commented Mar 22, 2020

This change was made 4 years ago, did you just upgrade now?

You're right that it wasn't discussed in the PR, and the default for this value is true.

I'd be concerned about the large number of exceptions enabling it would force people to add. Do you think this case happens often enough, where it's not tested whatsoever, that it'd be worth it?

I'd be content to switch to the exception pattern to resolve #1106, but I don't think that addresses your question.

@kevinoid
Copy link
Author

This change was made 4 years ago, did you just upgrade now?

Nope. I recently started invoking eslint with --report-unused-disable-directives and noticed the issue.

I'd be concerned about the large number of exceptions enabling it would force people to add. Do you think this case happens often enough, where it's not tested whatsoever, that it'd be worth it?

I think the rule is useful for my own projects, which is why I enabled the option in my own config. The only exceptions I've added are in tests which invoke constructors without new to test invalid invocations.

How common capitalized functions which should not be invoked with new are in Airbnb code and user code, and whether they comply with Airbnb style guidelines, I can't say. If you think the harm outweighs the gain, feel free to close.

Kevin

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

No branches or pull requests

4 participants
@ljharb @kevinoid and others