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

[legacy-framework] Add three lint rules to catch async/await promise bugs #662

Merged
merged 8 commits into from Jun 11, 2020

Conversation

flybayer
Copy link
Member

@flybayer flybayer commented Jun 10, 2020

What are the changes and their implications?

This adds three new lint rules:

  1. '@typescript-eslint/no-floating-promises': 'error'
  2. 'require-await': 'error'
  3. 'no-async-promise-executor': 'error'

These will help prevent various types of async/await promise bugs.

In fact, I think this may help with blitz-js/legacy-framework#931 and maybe even fix it

Checklist

  • Tests added for changes
  • PR submitted to blitzjs.com for any user facing changes

@blitzjs-bot blitzjs-bot bot added this to In Review in Dashboard Jun 10, 2020
@@ -64,7 +64,7 @@ transform.file = function transformFiles(
) {
const mergedOpts = Object.assign({}, defaultStreamOptions, options)
return through(mergedOpts, async function (item: PipelineItem, _, next: StreamApi['next']) {
processInput({transformFn, next, self: this, filesOnly: true, item})
await processInput({transformFn, next, self: this, filesOnly: true, item})
Copy link
Member Author

Choose a reason for hiding this comment

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

@ryardley is it possible this will fix #632 ??

Copy link
Collaborator

@ryardley ryardley Jun 10, 2020

Choose a reason for hiding this comment

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

Not sure. Streams are their own asynchronicity model and passing an async function as a transform doesn't have any effect on their behaviour as it is just a function that returns a promise chain instead of void. There is no try catch around the function passed to through so I don't see how this would change anything but maybe I could be wrong. Not sure if this means an error won't be swallowed or not. My favourite candidate for #632 is some kind of highwatermark issue but it is hard to investigate without reproducability. I am working on getting some kind of sensible debugging together to see if that can help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to fix it in my codebase.

@flybayer flybayer merged commit ea63de0 into canary Jun 11, 2020
Dashboard automation moved this from In Review to Done Jun 11, 2020
@flybayer flybayer deleted the eslint-promise branch June 11, 2020 02:03
@dillondotzip dillondotzip changed the title Add three lint rules to catch async/await promise bugs [legacy-framework] Add three lint rules to catch async/await promise bugs Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants