-
Notifications
You must be signed in to change notification settings - Fork 468
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
Remove duplicated info, improve clarity, add samples #1343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for this and the improvement. I have a few suggestions:
- Let's just use the title "Connection Pools" and remove the advanced management heading
- I really don't want the docs to include any hint of circumventing the ability to use parameterised queries. Too many devs pick up these docs and use them in production and query "helpers" (that pass queries around as strings into the library) are just something I want to avoid in the docs.
- I think you have a little bit of target fixation on the global pool and the errors you're seeing with connections being closed. These really are not related, when connect() is called repeatedly the pool and network connections are not touched in anyway and the examples you've given are just doing the same thing as the global pool but in a more complicated way that I wouldn't want to encourage users of the lib to do.
- I don't think the
CustomConnector
class is a good example / suggestion to be including in the docs; I don't think it adds value. Using a class for this is overkill, we should recommend using NodeJS's native caching ofrequire
calls to allow people to easily manage singletons, rather than just saying "Hey, here's a class, now you worry about managing it as a global singleton in your app" with not context or guidance about how to do that or what that means. - I'm not an express expert by any means, but the approach doesn't feel right to me. I think it's better to use the express app itself to pass around the pool. A proper, resilient implementation into express is out of scope of this library's documentation, though.
- The changes you've made to the example pool manager aren't for the best IMO. I've provided an example of what I think is better.
I appreciate you feel that the global connection pool (and repeated calls to connect()
) are causing your connections to be closed, the solution isn't to conflate the docs with that problem. If you're seeing connection issues I'd be happy to try to assist in helping with that. I run all my usage of this library exclusively in Function Apps and Azure SQL too, so this is not an environment or way of working that is unfamiliar to me.
Hey @trevorbye - are you happy to make the changes or shall I pick it up from here? |
What this does:
Improves clarity in the documentation around global pools, and adds some clarity/examples for using a single app-wide instance for the global pool to fix connection issues/anomalies. Even though there have been some fixes to allow repeated use of
sql.connect()
, it doesn't fix everything and there are still cases where a single instance is necessary/preferred depending on the implementation, environment, etc.There are many github issues on this that show that this is still an issue and needs to be better represented in the documentation. I was specifically having issues inside Azure Functions and with Azure SQL, but you can see in the newer discussion in #138 that people are also having issues with AWS lambda, heroku, etc.
There was also a lot of duplicated code (there were two different examples and sections for a custom pool implementation) and information in the pools documentation which was confusing and difficult to follow. I cleaned some of this up as well so this section is a lot more concise and easy to follow now.
Related issues:
#135
#118
#138
Pre/Post merge checklist: