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

Add eslint-plugin-node #268

Merged
merged 4 commits into from
Nov 28, 2017
Merged

Add eslint-plugin-node #268

merged 4 commits into from
Nov 28, 2017

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Nov 22, 2017

Fixes #123

Add the following rules:

Rule ID Description
no-unpublished-bin Disallow 'bin' files which are ignored by npm
no-unpublished-import Disallow import declarations of private things
no-unpublished-require Disallow require() expressions of private things
process-exit-as-throw Make process.exit() expressions the same code path as throw
no-deprecated-api Disallow unsupported ECMAScript features on the specified version
exports-style Enforce either module.exports or exports

The other rules provided are disabled for the following reasons:

Rule ID Reason for disabling
no-extraneous-import Redundant with import/no-extraneous-dependencies
no-extraneous-require Redundant with import/no-extraneous-dependencies
no-missing-import Redundant with import/no-unresolved
no-missing-require Redundant with import/no-unresolved
no-unsupported-features Doesn't allow to exclude compiled sources
shebang Doesn't exclude scripts executed with node but not referenced in "bin" (i.e main.js)
exports-style Too restrictive, forces to use either modules.exports or exports but not both

Let me know if I should make changes in the rules enabled/disabled or in the configuration.

@sindresorhus
Copy link
Member

no-unsupported-features | Doesn't allow to exclude compiled sources
-- | --

Can you open an issue on eslint-plugin-node about supporting this?

shebang | Doesn't exclude scripts executed with node but not referenced in "bin" (i.e main.js)
-- | --

This too.

exports-style | Too restrictive, forces to use either modules.exports or exports but not both
-- | --

And this.

// 'node/no-missing-import': 'error',
// 'node/no-missing-require': 'error',
'node/no-unpublished-bin': 'error',
'node/no-unpublished-import': 'error',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include electron and atom in the allowModules option: https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unpublished-import.md#allowmodules For ease of use with Electron and Atom.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

// 'node/no-missing-require': 'error',
'node/no-unpublished-bin': 'error',
'node/no-unpublished-import': 'error',
'node/no-unpublished-require': 'error',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@pvdlg pvdlg force-pushed the eslint-plugin-node branch 2 times, most recently from 1c7f225 to ed88204 Compare November 25, 2017 21:48
@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 25, 2017

no-unsupported-features | Doesn't allow to exclude compiled sources

mysticatea/eslint-plugin-node#97

shebang | Doesn't exclude scripts executed with node but not referenced in "bin" (i.e main.js)

mysticatea/eslint-plugin-node#96

exports-style | Too restrictive, forces to use either modules.exports or exports but not both

I first though it was possible to use both modules.exports and exports in the same project as long as it's consistent in the same js file. After more tests it turns out I was wrong.
Using export style that are consistent within each file but inconsistent across the project does create the issue described in the rule.
So I went ahead and enabled the rule

@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 27, 2017

@sindresorhus per this comment we could use the override feature to disable those rules for certain files.

In xo we could by default enable the shebang rule only for the files in the bin property of the package.json.

Regarding no-unsupported-features we could enable it only for certain files, but I don't know if there is a good default setting. We can either:

  • keep it disabled and let users enable it if they want to
  • Enable it and exclude ava files by default and let the users configure the overrides property to exclude more
  • Add a new property in xo called something like compiledSources that will allow to define an Array of globs. And we could use it to disable certain rules for files that match.

What's your thoughts ?
Also I'm not sure that should be part of this PR or if it should be implemented in a following one.

@sindresorhus
Copy link
Member

In xo we could by default enable the shebang rule only for the files in the bin property of the package.json.

Yes, let's wait an see the outcome of mysticatea/eslint-plugin-node#96

Regarding no-unsupported-features we could enable it only for certain files, but I don't know if there is a good default setting. We can either:

Let's keep it disabled. I don't want lots of complication for something that is not super useful with latest Node.js, as there are not that many commonly used unsupported features.

// Disabled as the rule doesn't allow to exclude compiled sources
// 'node/no-unsupported-features': 'error',
'node/process-exit-as-throw': 'error',
// Disabled as the rule doesn't exclude scripts executed with `node` but not referenced in "bin" (i.e main.js)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Disabled as the rule doesn't exclude scripts executed with `node` but not referenced in "bin" (i.e main.js)
// 'node/shebang': 'error',
'node/no-deprecated-api': 'error',
'node/exports-style': ['error', 'module.exports', {allowBatchAssign: true}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think allowBatchAssign should be true. It's not a good pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 28, 2017

Let's keep it disabled. I don't want lots of complication for something that is not super useful with latest Node.js, as there are not that many commonly used unsupported features.

This is something we can implement within the scope of #266

@sindresorhus
Copy link
Member

This is something we can implement within the scope of #266

Agreed :)

@sindresorhus sindresorhus merged commit 16fb8e2 into xojs:master Nov 28, 2017
@sindresorhus
Copy link
Member

sindresorhus commented Nov 28, 2017

Great! Thank you 🙌

Would you be interested in joining the project? You have done some high-quality contributions :)

@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 28, 2017

I'll take a stab at #266 sometime (in a weeks I guess).

@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 28, 2017

Would you be interested in joining the project? You done some high-quality contributions :)

Absolutely !

@pvdlg pvdlg deleted the eslint-plugin-node branch November 28, 2017 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint-plugin-node
2 participants