Skip to content

Implement client context#191

Merged
kravets-levko merged 4 commits intomainfrom
add-client-context
Oct 4, 2023
Merged

Implement client context#191
kravets-levko merged 4 commits intomainfrom
add-client-context

Conversation

@kravets-levko
Copy link
Copy Markdown
Contributor

@kravets-levko kravets-levko commented Oct 2, 2023

This library is designed so we have to initialize a root object (DBSQLClient) which later produces many of other object which represent sessions, operations, results sets and so on. And most of those objects require some common stuff, for example - logger, driver, connection - which are created and owned by their parent DBSQLClient instance. Previously we were passing those objects separately down the whole tree, and with time it became harder to add new items to that common context. In this PR, common objects are wrapped together into a context provider (implemented by DBSQLClient) which is then passed to all classes that need it.

This PR is also a spin-off of #174 (#193) which itself is an example why these changes are needed. There we need to access a HttpConnection instance owned by DBSQLClient in DBSQLSession and CloudFetchResult which required to pass this object down the tree.

Other use cases for this (not implemented in this PR, but planned for future) - add logging to classes which weren't capable to write to log until now (result handlers), and also move global config settings to context.

It may be easier to review changes commit by commit. Tests were just updated to match new signatures, without changes to their logic

Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
@kravets-levko kravets-levko temporarily deployed to azure-prod October 2, 2023 13:01 — with GitHub Actions Inactive
@kravets-levko kravets-levko mentioned this pull request Oct 3, 2023
5 tasks
Copy link
Copy Markdown

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Very good PR description. I've evaluated each commit sequentially. I'm not super-familiar with Node best practices but nothing here sticks out as a bad pattern and the changes make sense. This is good to merge assuming existing tests pass.

@kravets-levko kravets-levko merged commit 9eb3807 into main Oct 4, 2023
@kravets-levko kravets-levko deleted the add-client-context branch October 4, 2023 19:49
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.

2 participants