-
-
Notifications
You must be signed in to change notification settings - Fork 666
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
feat: add await option for next-tick-style rule #2508
base: master
Are you sure you want to change the base?
Conversation
callbackBody = | ||
args.type === 'ArrowFunctionExpression' || | ||
args.type === 'FunctionExpression' | ||
? extractCallbackBody(args, sourceCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't extract callback body directly, there might be variable conflicts.
export default {
async created() {
const foo = 1;
nextTick(() => {
const foo = 2;
doSomething(foo);
})
}
}
lib/rules/next-tick-style.js
Outdated
if (callback.body.type === 'BlockStatement') { | ||
return sourceCode | ||
.getText(callback.body) | ||
.slice(1, -1) // Remove curly braces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can simply keep the block.
lib/rules/next-tick-style.js
Outdated
|
||
callbackBody = | ||
args.type === 'ArrowFunctionExpression' || | ||
args.type === 'FunctionExpression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args.type === 'FunctionExpression' | |
(args.type === 'FunctionExpression' && !args.geneator) |
lib/rules/next-tick-style.js
Outdated
const sourceCode = context.getSourceCode() | ||
|
||
// Handle callback to await conversion | ||
if (callExpression.arguments.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (callExpression.arguments.length > 0) { | |
if (callExpression.arguments.length === 1) { |
) | ||
return fixer.replaceText( | ||
callExpression.parent, | ||
`await ${nextTickCaller}();${callbackBody};` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need make sure the nextTick
call is an expression statement, otherwise this will cause sytax errors.
if (foo) nextTick(...)
(() => nextTick(...))();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fisker How would you suggest the fix method handle these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore or turn into a block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ignore" means no fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've added a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it related to this one: #2508 (comment) ?
I removed the .slice()
method which keeps the block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry didn't see that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Is everything good to go now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a maintainer..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems not the correct fix.. It changes behavior
nextTick(() => console.log(2))
console.log(1)
Logs 1 2
await nextTick(); console.log(2)
console.log(1)
Logs 2 1
Maybe use suggestions instead?
lib/rules/next-tick-style.js
Outdated
function isAwaitedFunction(callExpression) { | ||
return ( | ||
callExpression.parent.type === 'AwaitExpression' && | ||
callExpression.parent.parent.type !== 'MemberExpression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this .parent.parent
check needed.
if (callback.body.type === 'BlockStatement') { | ||
return sourceCode.getText(callback.body).trim() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this branch.
args.type === 'ArrowFunctionExpression' || | ||
(args.type === 'FunctionExpression' && !args.generator) | ||
? extractCallbackBody(args, sourceCode) | ||
: `${sourceCode.getText(args)}()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not safe
nextTick(foo || bar)
- await nextTick(); foo || bar()
+ await nextTick(); (foo || bar)()
If foo
is truly, it will not be called
return sourceCode.getText(callback.body).trim() | ||
} | ||
|
||
return sourceCode.getText(callback.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function body can't exact directly if it has return
const foo = async () => {
nextTick(() => {
return;
})
return false
}
returns false
const foo = async () => {
await nextTick(); {
return;
}
return false
}
returns undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also broken if the callback has .id
or used arguments
inside.
const foo = async () => {
nextTick(function foo() {
use(foo)
use(arguments)
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses #2485