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
merged 5 commits into from Jul 23, 2018

Conversation

Projects
None yet
2 participants
@vladimiry
Copy link
Contributor

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/build.sh 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 some commits Jul 22, 2018

missed "queryFilter" error handling
* prevents uncaught promise error happening
improve "_NanoSQLQuery.exec" method signature
* 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

@ClickSimply ClickSimply merged commit 89ca4dc into ClickSimply:master Jul 23, 2018

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jul 23, 2018

This PR is live on NPM as of 1.7.2.

@vladimiry

This comment has been minimized.

Copy link
Contributor Author

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
You can’t perform that action at this time.