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
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions .eslintrc.js
Expand Up @@ -6,20 +6,24 @@ module.exports = {
ecmaFeatures: {
jsx: true,
},
project: `./tsconfig.json`,
},
plugins: ['@typescript-eslint', 'import', 'unicorn'],
extends: ['react-app'],
rules: {
'react/react-in-jsx-scope': 'off', // React is always in scope with Blitz
'jsx-a11y/anchor-is-valid': 'off', //Doesn't play well with Blitz/Next <Link> usage
'import/first': 0,
'import/no-default-export': ['error'],
'import/first': 'off',
'import/no-default-export': 'error',
'require-await': 'error',
'no-async-promise-executor': 'error',
'unicorn/filename-case': [
'error',
{
case: 'kebabCase',
},
],
'@typescript-eslint/no-floating-promises': 'error',
},
ignorePatterns: ['packages/cli/', 'packages/generator/templates'],
overrides: [
Expand All @@ -28,6 +32,7 @@ module.exports = {
rules: {
'import/no-default-export': 'off',
'unicorn/filename-case': 'off',
'@typescript-eslint/no-floating-promises': 'off',
},
},
],
Expand Down
4 changes: 2 additions & 2 deletions package.json
Expand Up @@ -120,15 +120,15 @@
"husky": {
"hooks": {
"pre-commit": "lint-staged && pretty-quick --staged",
"pre-push": "yarn lint",
"post-checkout": "if [[ $HUSKY_GIT_PARAMS =~ 1$ ]]; then yarn install --frozen-lockfile; fi",
"post-merge": "yarn install --frozen-lockfile",
"post-rebase": "yarn install"
}
},
"lint-staged": {
"*.{js,ts,tsx}": [
"eslint --fix",
"git add"
"eslint --fix"
]
},
"resolutions": {
Expand Down
2 changes: 1 addition & 1 deletion packages/blitz/test/todo.test.ts
@@ -1,3 +1,3 @@
it('todo', async () => {
it.skip('todo', () => {
expect(true).toBe(true)
})
1 change: 1 addition & 0 deletions packages/core/src/rpc.ts
Expand Up @@ -33,6 +33,7 @@ export async function executeRpcCall(url: string, params: any, opts: Options = {

executeRpcCall.warm = (url: string) => {
if (typeof window !== 'undefined') {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
window.fetch(url, {method: 'HEAD'})
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/utils/query-cache.ts
Expand Up @@ -7,6 +7,6 @@ export interface QueryCacheFunctions<T> {
export const getQueryCacheFunctions = <T>(queryKey: string): QueryCacheFunctions<T> => ({
mutate: (newData) => {
queryCache.setQueryData(queryKey, newData)
queryCache.refetchQueries(queryKey, {force: true})
return queryCache.refetchQueries(queryKey, {force: true})
},
})
4 changes: 2 additions & 2 deletions packages/core/test/rpc.test.ts
Expand Up @@ -10,7 +10,7 @@ declare global {

describe('RPC', () => {
describe('HEAD', () => {
it('warms the endpoint', async () => {
it('warms the endpoint', () => {
expect.assertions(1)
executeRpcCall.warm('/api/endpoint')
expect(global.fetch).toBeCalled()
Expand All @@ -37,7 +37,7 @@ describe('RPC', () => {
}
})

it('handles errors', async () => {
it('handles errors', () => {
expect.assertions(1)
const fetchMock = jest
.spyOn(global, 'fetch')
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/use-query.test.tsx
Expand Up @@ -6,6 +6,7 @@ describe('useQuery', () => {
const setupHook = (params: any, queryFn: (...args: any) => Promise<any>): [{data?: any}, Function] => {
let res = {}
function TestHarness() {
// eslint-disable-next-line require-await
useQuery(async (num: number) => num, 1)
const [data] = useQuery(queryFn, params)
Object.assign(res, {data})
Expand All @@ -23,6 +24,7 @@ describe('useQuery', () => {
}

describe('a "query" that converts the string parameter to uppercase', () => {
// eslint-disable-next-line require-await
const upcase = async (args: string): Promise<string> => {
return args.toUpperCase()
}
Expand All @@ -44,6 +46,7 @@ describe('useQuery', () => {
const [res, rerender] = setupHook(() => params(), upcase)
await screen.findByText('Missing Dependency')

// eslint-disable-next-line require-await
await act(async () => {
// simulates the dependency becoming available
params = () => 'test'
Expand Down
4 changes: 2 additions & 2 deletions packages/file-pipeline/src/helpers/agnostic-source/index.ts
Expand Up @@ -84,8 +84,8 @@ export function agnosticSource({ignore, include, cwd, watch: watching = false}:
})
const close = () => watcher.fswatcher.close()

stream.on('end', () => {
close()
stream.on('end', async () => {
await close()
})

return {stream, close}
Expand Down
22 changes: 10 additions & 12 deletions packages/file-pipeline/src/helpers/writer/write.test.ts
Expand Up @@ -21,17 +21,15 @@ describe('writer', () => {
writer.write(new File({path: normalize('/thing'), event: 'add'}))
writer.write('foo')

Promise.all([
await testStreamItems(
bus,
[
// Note order is not necessarily deterministic
// If that is the case we can change the way we test this
{type: FILE_WRITTEN, file: normalize('/thing')},
{type: FILE_DELETED, file: normalize('/bar')},
],
({type, payload}) => ({type, file: payload.path}),
),
])
await testStreamItems(
bus,
[
// Note order is not necessarily deterministic
// If that is the case we can change the way we test this
{type: FILE_WRITTEN, file: normalize('/thing')},
{type: FILE_DELETED, file: normalize('/bar')},
],
({type, payload}) => ({type, file: payload.path}),
)
})
})
2 changes: 1 addition & 1 deletion packages/file-pipeline/src/transform.test.ts
Expand Up @@ -46,7 +46,7 @@ describe('transform', () => {
)
})

it('should swallow stuff when it doesnt return anything', async () => {
it('should swallow stuff when it doesnt return anything', () => {
const s = transform.file((_, {next}) => next())
const l: any[] = []
s.pipe(
Expand Down
4 changes: 2 additions & 2 deletions packages/file-pipeline/src/transform.ts
Expand Up @@ -54,7 +54,7 @@ export function transform(
) {
const mergedOpts = Object.assign({}, defaultStreamOptions, options)
return through(mergedOpts, async function (item: PipelineItem, _, next: StreamApi['next']) {
processInput({transformFn, next, self: this, item})
await processInput({transformFn, next, self: this, item})
})
}

Expand All @@ -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.

})
}

Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/app-generator.ts
Expand Up @@ -39,6 +39,7 @@ export class AppGenerator extends Generator<AppGeneratorOptions> {
return ''
}

// eslint-disable-next-line require-await
async preCommit() {
this.fs.move(this.destinationPath('gitignore'), this.destinationPath('.gitignore'))
}
Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/form-generator.ts
Expand Up @@ -26,6 +26,7 @@ export class FormGenerator extends Generator<FormGeneratorOptions> {
return `[${input}]`
}

// eslint-disable-next-line require-await
async getTemplateValues() {
return {
parentModelId: this.getId(this.options.parentModel),
Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/model-generator.ts
Expand Up @@ -21,6 +21,7 @@ export class ModelGenerator extends Generator<ModelGeneratorOptions> {
return ''
}

// eslint-disable-next-line require-await
async write() {
try {
if (!this.fs.exists(path.resolve('db/schema.prisma'))) {
Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/mutation-generator.ts
Expand Up @@ -26,6 +26,7 @@ export class MutationGenerator extends Generator<MutationGeneratorOptions> {
return `[${input}]`
}

// eslint-disable-next-line require-await
async getTemplateValues() {
return {
parentModelId: this.getId(this.options.parentModel),
Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/page-generator.ts
Expand Up @@ -26,6 +26,7 @@ export class PageGenerator extends Generator<PageGeneratorOptions> {
return `[${input}]`
}

// eslint-disable-next-line require-await
async getTemplateValues() {
return {
parentModelId: this.getId(this.options.parentModel),
Expand Down
1 change: 1 addition & 0 deletions packages/generator/src/generators/query-generator.ts
Expand Up @@ -26,6 +26,7 @@ export class QueryGenerator extends Generator<QueryGeneratorOptions> {
return `[${input}]`
}

// eslint-disable-next-line require-await
async getTemplateValues() {
return {
parentModelId: this.getId(this.options.parentModel),
Expand Down
3 changes: 1 addition & 2 deletions packages/generator/templates/app/package.json
Expand Up @@ -23,8 +23,7 @@
},
"lint-staged": {
"*.{js,ts,tsx}": [
"eslint --fix",
"git add"
"eslint --fix"
]
},
"dependencies": {
Expand Down
6 changes: 4 additions & 2 deletions packages/installer/src/executors/add-dependency-executor.tsx
Expand Up @@ -83,15 +83,15 @@ export const Propose: Executor['Propose'] = ({cliArgs, step, onProposalAccepted}
)
}

async function getPackageManager(): Promise<'yarn' | 'npm'> {
function getPackageManager(): Promise<'yarn' | 'npm'> {
if (fs.existsSync(path.resolve('package-lock.json'))) {
return 'npm'
}
return 'yarn'
}

async function installPackages(packages: NpmPackage[], isDev = false) {
const packageManager = await getPackageManager()
const packageManager = getPackageManager()
const args: string[] = ['add']

if (isDev) {
Expand Down Expand Up @@ -128,6 +128,7 @@ export const Commit: Executor['Commit'] = ({cliArgs, step, onChangeCommitted}) =
await installPackages(packagesToInstall)
setDepsInstalled(true)
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
installDeps()
}, [cliArgs, step])

Expand All @@ -140,6 +141,7 @@ export const Commit: Executor['Commit'] = ({cliArgs, step, onChangeCommitted}) =
await installPackages(packagesToInstall, true)
setDevDepsInstalled(true)
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
installDevDeps()
}, [cliArgs, depsInstalled, step])

Expand Down
2 changes: 1 addition & 1 deletion packages/installer/src/executors/file-prompt.ts
Expand Up @@ -13,7 +13,7 @@ interface FilePromptOptions {
context: any
}

async function getMatchingFiles(filter: string = ''): Promise<string[]> {
function getMatchingFiles(filter: string = ''): Promise<string[]> {
return globby(filter, {expandDirectories: true})
}

Expand Down
Expand Up @@ -37,6 +37,7 @@ export const Propose: Executor['Propose'] = ({cliArgs, onProposalAccepted, step}
const newFile = processFile(originalFile, (step as Config).transform)
return createPatch(fileToTransform, originalFile, newFile)
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
generateDiff().then(setDiff)
}, [cliArgs, step])

Expand Down
2 changes: 2 additions & 0 deletions packages/installer/src/executors/new-file-executor.tsx
Expand Up @@ -73,6 +73,7 @@ export const Propose: Executor['Propose'] = ({cliArgs, onProposalAccepted, step}
setDryRunOutput(results)
}
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
proposeFileAdditions()
}, [dryRunOutput, generatorArgs])

Expand Down Expand Up @@ -117,6 +118,7 @@ export const Commit: Executor['Commit'] = ({cliArgs, onChangeCommitted, step}) =
setFileCreateOutput(results)
}
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
createNewFiles()
}, [fileCreateOutput, generatorArgs])

Expand Down
3 changes: 3 additions & 0 deletions packages/installer/src/installer.tsx
Expand Up @@ -37,12 +37,15 @@ export class Installer<Options extends RecipeMeta> {
setupTsNode()
}

// eslint-disable-next-line require-await
private async validateArgs(cliArgs: {}): Promise<void> {
if (this.options.validateArgs) return this.options.validateArgs(cliArgs)
}
// eslint-disable-next-line require-await
private async preInstall(): Promise<void> {
if (this.options.preInstall) return this.options.preInstall()
}
// eslint-disable-next-line require-await
private async postInstall(): Promise<void> {
if (this.options.postInstall) return this.options.postInstall()
}
Expand Down
10 changes: 5 additions & 5 deletions packages/repl/src/repl.ts
Expand Up @@ -14,25 +14,25 @@ const projectRoot = pkgDir.sync() || process.cwd()
const commands = {
reload: {
help: 'Reload all modules',
action(this: REPLServer) {
async action(this: REPLServer) {
this.clearBufferedCommand()
console.log('Reloading all modules...')
loadModules(this)
await loadModules(this)
this.displayPrompt()
},
},
}

const runRepl = async (replOptions: REPL.ReplOptions) => {
const repl = initializeRepl(replOptions)
const repl = await initializeRepl(replOptions)
await setupFileWatchers(repl)
}

const initializeRepl = (replOptions: REPL.ReplOptions) => {
const initializeRepl = async (replOptions: REPL.ReplOptions) => {
const repl = REPL.start(replOptions)

defineCommands(repl, commands)
loadModules(repl)
await loadModules(repl)
setupHistory(repl)

return repl
Expand Down
2 changes: 1 addition & 1 deletion packages/server/src/manifest-loader.ts
Expand Up @@ -2,7 +2,7 @@ import {readFile} from 'fs'
import {Manifest} from './stages/manifest'

export const ManifestLoader = {
async load(filename: string) {
load(filename: string) {
return new Promise((resolve, reject) => {
readFile(filename, 'utf8', (err, data) => {
if (err) {
Expand Down
2 changes: 1 addition & 1 deletion packages/server/src/next-utils.ts
Expand Up @@ -51,7 +51,7 @@ export async function nextStartDev(
})
}

export async function nextBuild(nextBin: string, cwd: string) {
export function nextBuild(nextBin: string, cwd: string) {
return new Promise((res, rej) => {
spawn(nextBin, ['build'], {
cwd,
Expand Down