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

feat: add the ability to pass the driver into all database types #8259

Merged
merged 2 commits into from Oct 22, 2021

Conversation

dncrews
Copy link
Contributor

@dncrews dncrews commented Oct 9, 2021

Description of change

Fixes #5211

This PR introduces a new configuration parameter for all of the drivers that allows the developer to pass in their own client library (as they can now with some of the drivers). This allows for the added ability to be able to wrap the clients with telemetry or to pass in a test client.

Notes

  • Even with Docker I wasn't able to get tests to pass locally (before making my changes). I am running tests here in the PR, as that's where I can get them to run. Perhaps there is more documentation to write, or I'm just not familiar with the toolset enough. Either way, I'm open to suggestions where I can help.
  • I'll accept any recommendations about documentation or tests that should be written before merging
  • I wasn't sure if the || syntax was preferred or if using a full if statement was desired, so I went with the former, and I'm willing to change to the latter.
  • Several of the other changes made are just my editor fixing inconsistent linting issues. I'm willing to revert those if desired to keep focus on my actual changes.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change (open to suggestions)
  • Documentation has been updated to reflect this change (open to suggestions)
  • The new commits follow conventions explained in CONTRIBUTING.md

@@ -125,7 +125,8 @@ export class BetterSqlite3Driver extends AbstractSqliteDriver {
*/
protected loadDependencies(): void {
try {
this.sqlite = PlatformTools.load("better-sqlite3");
const sqlite = (this.connection.options as BetterSqlite3ConnectionOptions).driver || PlatformTools.load("better-sqlite3");
Copy link
Contributor

Choose a reason for hiding this comment

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

why not this.options

Copy link
Contributor Author

@dncrews dncrews Oct 11, 2021

Choose a reason for hiding this comment

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

I didn't know about that. That's way easier and wouldn't require the type casting. I'll do that, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f9bdd18

@imnotjames imnotjames added the hacktoberfest-accepted label hacktoberfest label Oct 10, 2021
@pleerock
Copy link
Member

Seems good. Thank you for contribution!

@pleerock pleerock merged commit 2133ffe into typeorm:master Oct 22, 2021
@dncrews dncrews deleted the feat/driver-configuration branch October 27, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted label hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use an existing pg.Client connection
3 participants