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

rule new-cap problem #892

Closed
maticrivo opened this issue May 23, 2017 · 19 comments

Comments

@maticrivo
Copy link

commented May 23, 2017

Hey, i use the package hapijs/boom for server errors and all the methods are with camelCase for example new Boom.notImplemented()
standard is throwing an error

A constructor name should not start with a lowercase letter  new-cap

but i can't disable each line individually, is there a way to disable all new-cap warnings? or maybe fix the rule so it won't throw an error?

@Flet

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

adding This to the top of the file should work:

/* eslint-disable new-cap */

Looking at the new-cap rule, there is an additional option called properties that can be set to false to allow this.

Right now we don't specify that option in eslint-config-standard so it falls back to the default, which is to ensure object properties are capitalized.

Does anyone have any thoughts on changing the new-cap rule to allow object properties to be lowercase?

@LinusU

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

Would personally prefer to have properties: true. I don't think that is's a good (nor widespread) convention to have anything that should be newed start with a lower case letter...

@jasonkarns

This comment has been minimized.

Copy link

commented Dec 27, 2017

Not sure if it relates to the properties bit, but if it fixes this, I'm all for it:

new (require('foo').default)() -> (Error: "A constructor name should not start with a lowercase letter. (new-cap)")

Actually, there's the counter-example, @LinusU: it's quite common for a default export to be a constructor function. But one is clearly not able to control the capitalization of the ESM-as-CJS default export.

@LinusU

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

removed spam from lordspecter13

@standard standard deleted a comment Dec 27, 2017

@standard standard deleted a comment Dec 27, 2017

@stale

This comment has been minimized.

Copy link

commented May 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label May 10, 2018

@stale stale bot closed this May 17, 2018

@feross

This comment has been minimized.

Copy link
Member

commented May 20, 2018

@LinusU Do you have thoughts on @jasonkarns's last comment regarding it being common for a default export to be a constructor function?

@feross feross reopened this May 20, 2018

@stale stale bot removed the stale label May 20, 2018

@feross feross added the enhancement label May 20, 2018

@jkga

This comment has been minimized.

Copy link

commented Jan 17, 2019

Any update on this?

@LinusU

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Missed answering to the previous comment, so here goes: I still think that that isn't used too much, wouldn't one do something like this in those cases? 🤔

const Foobar = require('foobar').default

const blah = new Foobar()
@jkga

This comment has been minimized.

Copy link

commented Jan 17, 2019

@LinusU but its also being used with dynamic import

return import('./components/my-component').then(dyamicLoadedClass => { 
      return new dyamicLoadedClass.default()
      ...
})
@LinusU

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Maybe we could make an exception for .default 🤔

@dcsan

This comment has been minimized.

Copy link

commented Mar 6, 2019

yeah i have exact same problem on a Pusher library:

Chatkit.default({

is there a way to disable linting on just a single line and not the whole file?

@maticrivo

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

You can do it by adding a comment like // eslint-disable new-cap to the end of the line

@dcsan

This comment has been minimized.

Copy link

commented Mar 6, 2019

hmm doesn't seem to work for me

image

image

@jkga

This comment has been minimized.

Copy link

commented Mar 6, 2019

@dcsan Try to put it on the very first line of the file

@dcsan

This comment has been minimized.

Copy link

commented Mar 6, 2019

this works at the top of the file but i just wanted to disable it for one line using the external lib call, not my own code.

/* eslint-disable new-cap */

@maticrivo

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Try with // eslint-disable-line new-cap at the end of the line or // eslint-disable-next-line new-cap one line above the code you want eslint to disable

@feross

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

It's always better to always assign the name default to a name with a capitalized first letter before calling the constructor. It should always be possible to do this. For example, @jkga's code sample can be modified like this:

return import('./components/my-component').then(dyamicLoadedClass => { 
      const MyClass = dyamicLoadedClass.default
      return new MyClass()
      ...
})

Is there some reason that I'm missing why this won't work? @LinusU do you have thoughts?

We could change the rule to this:

"new-cap": ["error", { "newIsCap": true, "capIsNew": false, "properties": true, "newIsCapExceptions": ["default"] }]

which would allow the original code sample to work:

return import('./components/my-component').then(dyamicLoadedClass => { 
      return new dyamicLoadedClass.default()
      ...
})

But can someone make an argument for a situation where this is more desirable?

@LinusU

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I personally still feel that you should always reassign it to a variable with a capitalised name, but I don't feel too strongly about it...

const Foo = require('foo').default

new Foo()
import('./components/my-component').then(module => {
  const DyamicLoadedClass = module.default

  new DyamicLoadedClass()
})
import('./components/my-component').then(({ default: DyamicLoadedClass }) => {
  new DyamicLoadedClass()
})
@feross

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@LinusU Agreed. Let's not add an exception for this. If others can raise valid reasons, happy to reconsider.

@feross feross closed this Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.