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

Improve "exec" method signature consistency and prevent "queryFilter" uncaught promise error happening #73

merged 5 commits into from Jul 23, 2018

Improve "exec" method signature consistency and prevent "queryFilter" uncaught promise error happening #73

merged 5 commits into from Jul 23, 2018


Copy link

@vladimiry vladimiry commented Jul 22, 2018

What has been changed:

  • Added promise rejection handling for queryFilter handler. I'm getting uncaught promise error without that. This is a primary reason why I'm putting this PR.
  • Set _NanoSQLQuery.exec method's return signature to Promise and enabled minor promise errors catching tweaking. Generally, if return is a Promise, then it's recommended to define it explicitly as otherwise, projects using a library will not be able to take advantage of using await-promise / no-floating-promises like tslint rules. In some cases calling a promise-returning function without await on it can be a very destructive case and it's hard to notice such flows without automated code analyzing. Named tslint rules help to prevent it from happening.
  • Added pipeline-like dist npm script as an example of combining scripts to a flow. This script for example can be enabled for running in CI environemnt. Btw, I believe it's possible to ditch bin/ completely expressing everything from there just in the form of npm scripts (there are rimraf and cpx npm modules for example).
  • Updated dev dependencies.

Important note. I didn't update generated stuff, like lib / docs / etc. If PR is going to be merged, you will need to re-generate stuff after the merging before releasing (see mentioned above dist npm script).

What I believe could, in general, improve this library robustness and consistency:

  • Enabling at least await-promise, no-floating-promises and no-unused-imports tslint rules. Ideally typedef rule as well, forcing at least always putting explicit return signature, which is a good at least for library-like projects. But enabling typedef rule would cause a really huge bunch of cases to handle. Currently, there are a lot of places in the code which can potentially lead to the uncaught promises rejections happening, and queryFilter is just one of them. Named tslint rules can help with preventing it from happening.
  • Moving to async/await from raw promises. That would among other benefits force you to explicitly define Promise in return signature, which is a good thing.
  • Replacing callbacks with promises everywhere where it's possible and makes sense.
  • Enabling polyfill for promises globally once somewhere in module entry point code, and then just using Promise as a global thing everywhere (which is described in node_modules/typescript/lib/lib.es5.d.ts), not importing it from src/utilities.ts.
  • Enabling strict flag in tsconfig.json (grouping flag) and disabling specific restrictions as less as possible.
vladimiry added 5 commits Jul 22, 2018
* prevents uncaught promise error happening
* Indicate clearly that _NanoSQLQuery.exec returns a Promise. Helpful for project which go with await-promise/no-floating-promises tslint rules enabled.
@vladimiry vladimiry changed the title Improve exec method consistency Improve "exec" method signature consistency and prevent "queryFilter" uncaught promise error happening Jul 22, 2018
@only-cliches only-cliches merged commit 89ca4dc into only-cliches:master Jul 23, 2018
Copy link

@only-cliches only-cliches commented Jul 23, 2018

This PR is live on NPM as of 1.7.2.

Copy link
Contributor Author

@vladimiry vladimiry commented Jul 23, 2018

I confirm that with 1.7.2 queryFilter works as expected in my case, no uncaught promise rejections anymore, thanks Scott.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants