feat(rule): add no-new-statics rule #82
Merged
+107
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
macklinu
commented
Feb 5, 2018
index.js
Outdated
| @@ -32,6 +35,7 @@ module.exports = { | |||
| 'promise/no-promise-in-callback': 'warn', | |||
| 'promise/no-callback-in-promise': 'warn', | |||
| 'promise/avoid-new': 'warn', | |||
| 'promise/no-new-statics': 'warn', | |||
There was a problem hiding this comment.
I added this rule to the recommended configuration as a warning - does this need adjustment?
There was a problem hiding this comment.
I'm thinking this should actually be an error. Playing around in a Node REPL:
❯ node
> new Promise.resolve()
TypeError: Promise.resolve is not a constructor
rules/lib/is-promise.js
Outdated
| @@ -2,37 +2,35 @@ | |||
| * Library: isPromise | |||
| * Makes sure that an Expression node is part of a promise. | |||
| */ | |||
| var STATIC_METHODS = [ | |||
There was a problem hiding this comment.
I extracted this array into its own module so that I could use it in multiple files.
rules/lib/is-promise.js
Outdated
|
|
||
| 'use strict' | ||
|
|
||
| const PROMISE_STATICS = require('./promise-statics') |
There was a problem hiding this comment.
Open to a better file/module name than promise-statics if you have suggestions!
rules/no-new-statics.js
Outdated
| ) { | ||
| context.report({ | ||
| node, | ||
| message: 'Avoid calling new on a Promise static method' |
There was a problem hiding this comment.
This is a generic message right now that helps get the point across (I think). Improvement suggestions welcome.
macklinu
force-pushed
the
no-new-statics
branch
7 times, most recently
from
32ac83a to
d7ab89e
Compare
macklinu
force-pushed
the
no-new-statics
branch
2 times, most recently
from
7b84d75 to
40796b5
Compare
macklinu
force-pushed
the
no-new-statics
branch
from
40796b5 to
ad9c7f8
Compare
8 tasks
|
Ready to merge if CI passes! |
This was referenced Mar 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #75
Hoping to continue discussion from #75 to solidify this feature.
For now, I've created a new rule named
no-new-statics, which warns if you call any of the Promise static methods withnew. I'm open to new naming or including it in another rule, if that would be more appropriate. Feedback appreciated. 😄