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

Promise callback with queryConfig options. #337

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Conversation

nmaves
Copy link
Contributor

@nmaves nmaves commented Sep 16, 2020

Resolves #335

When you execute a PG query with an QueryConfig object the implicit promise callback is the 3 element in the args.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @nmaves! Any chance you could add a unit test to verify this behavior when capturing a query with a promise-based callback? Also, what would the 2nd argument be then in this case?

@nmaves
Copy link
Contributor Author

nmaves commented Sep 17, 2020

Honestly, I am not sure how to write this with your current test setup.

From my testing that second argument is null. I put in debug statements with your code and found that to be the case.

@willarmiros
Copy link
Contributor

That's fair enough, I'd be fine with not having a unit test here if there was some validation to check if the 3rd argument is a function. Something similar to what we already do on L45: https://github.com/aws/aws-xray-sdk-node/blob/master/packages/postgres/lib/postgres_p.js#L45

@nmaves
Copy link
Contributor Author

nmaves commented Sep 22, 2020

Does that work?

@willarmiros
Copy link
Contributor

willarmiros commented Sep 22, 2020

Looks like the test is failing because you need to include argsObj[0].callback instead of undefined as the final fallback possibility to match the original behavior.

…romise callback is the 3 element in the args.
@nmaves
Copy link
Contributor Author

nmaves commented Sep 23, 2020

The test is now passing.

@willarmiros
Copy link
Contributor

Great, thanks again for the changes!

@willarmiros willarmiros merged commit 2d117b2 into aws:master Sep 23, 2020
@nmaves
Copy link
Contributor Author

nmaves commented Nov 13, 2020

hey @willarmiros you guys think you might snap a release anytime soon to get this out in the wild?

@willarmiros
Copy link
Contributor

We're definitely due for a release, I'll prioritize one soon. Thanks for the ping @nmaves !

@nmaves
Copy link
Contributor Author

nmaves commented Jan 18, 2021

@willarmiros don't make me beg and look like a need open source abuser :)

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

Successfully merging this pull request may close these issues.

PG capture does not work well with queryConfig objects.
2 participants