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

Generic client, Browser + Node.js connections #165

Merged
merged 37 commits into from
Jul 13, 2023
Merged

Generic client, Browser + Node.js connections #165

merged 37 commits into from
Jul 13, 2023

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Jun 8, 2023

Summary

Further efforts after #163 (different approach).

Introduce browser connection (using native fetch) with no Node.js modules in the common interfaces. No polyfills are required. It will also be possible to write new connections on top of @clickhouse/client-common if necessary.

The client was refactored into three packages:

  • @clickhouse/client-common: all possible platform-independent code, types and interfaces
  • @clickhouse/client-browser: new "browser" (or non-Node.js env) connection, uses native fetch. Depends on the common package.
  • @clickhouse/client: Node.js connection as it was before. Depends on the common package.

Related issues:

Additionally, resolves #2, #17

Jest was replaced with jasmine + jasmine-expect, cause it's easier to maintain the tests that way - Karma does not like Jest very much; however, one of few downsides of that approach is that retryOnFailure does not work anymore as jasmine matchers do not throw an exception, thus in some tests I had to use janky hardcoded sleep calls.

Current browser connection limitations:

  • Browser connection insert streams won't be supported atm because it does not work across all the platforms (see FF issue); arrays work fine though; select streams are also fine;
  • Custom Logger currently is not implemented for the browser connection;
  • Custom user-agent (with application name) is not implemented for browser connection (need to investigate what's actually possible there. Currently, I don't set it at all).

Checklist

  • Unit and integration tests covering the common scenarios were added

Copy link
Member

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

LGTM overall. What are our next steps?
There are a few follow-ups to address, at least a simple browser test for memory leaks, update docs, release beta versions, and gather feedback. Did I miss anything?

README.md Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
tsconfig.dev.json Outdated Show resolved Hide resolved
packages/client-common/src/logger.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
karma.config.cjs Show resolved Hide resolved
__tests__/integration/browser/browser_exec.test.ts Outdated Show resolved Hide resolved
__tests__/utils/client.ts Outdated Show resolved Hide resolved
__tests__/integration/data_types.test.ts Outdated Show resolved Hide resolved
__tests__/integration/config.test.ts Outdated Show resolved Hide resolved
@slvrtrn slvrtrn changed the base branch from 0.1.0 to 0.2.0 June 22, 2023 13:49
@slvrtrn
Copy link
Contributor Author

slvrtrn commented Jul 13, 2023

@mshustov, I added several follow-ups to reduce the scope of this one (sorted by priority).

Before releasing the first beta (0.2.0-beta1):

After the first beta:

After "stable" 0.2.0:

Please let me know if I am missing something.

CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Mikhail Shustov <mikhail.shustov@clickhouse.com>
@slvrtrn slvrtrn merged commit 9da7655 into 0.2.0 Jul 13, 2023
@slvrtrn slvrtrn deleted the generic-client branch July 13, 2023 16:41
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