Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork680
feat: add await option for next-tick-style rule#2508
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
exportdefault{asynccreated(){constfoo=1;nextTick(()=>{constfoo=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 thenextTick 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)
Logs1 2
awaitnextTick();console.log(2)console.log(1)
Logs2 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)()
Iffoo 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 hasreturn
constfoo=async()=>{nextTick(()=>{return;})returnfalse}
returnsfalse
constfoo=async()=>{awaitnextTick();{return;}returnfalse}
returnsundefined
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 usedarguments inside.
constfoo=async()=>{nextTick(functionfoo(){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