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

Remove platform-specific dependencies #162

Closed
maxprilutskiy opened this issue May 23, 2023 · 10 comments
Closed

Remove platform-specific dependencies #162

maxprilutskiy opened this issue May 23, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@maxprilutskiy
Copy link

Issue Description:

I would like to raise a concern regarding the current state of the @clickhouse/client package.

It heavily relies on platform-specific dependencies, including http, zlib, and stream, which limits its usability in certain environments, particularly serverless edge setups. I believe it is crucial to address this issue and propose a solution that would make @clickhouse/client more versatile and compatible with a broader range of platforms.

Problem Statement:

The presence of node-specific dependencies in @clickhouse/client prevents its usage in environments where these dependencies are either unavailable or incompatible. For instance, serverless edge environments require platform-agnostic libraries to ensure seamless execution across different platforms. Unfortunately, the current state of @clickhouse/client hinders its potential in such scenarios.

Desired Outcome:

I kindly request the development team to reconsider the dependencies in @clickhouse/client and strive for a solution that eliminates any platform-specific ties. By doing so, we can make the client package more adaptable, enabling it to be used in a wider array of environments, including serverless edge deployments. This change would empower developers to leverage the benefits of the ClickHouse client in scenarios where these platform-specific dependencies are not available or compatible.

Proposed Solution:

To address this issue effectively, I propose refactoring @clickhouse/client to remove direct dependencies on node-specific packages like http, zlib, and stream. Instead, I recommend employing platform-agnostic alternatives or providing a pluggable interface that allows users to supply their own implementations for the underlying communication layer. Or, fetch API could just be used instead, that is supported everywhere nowadays.

Benefits:

  • Enhanced compatibility: Eliminating platform-specific dependencies will broaden the compatibility of @clickhouse/client, making it viable for a wider range of platforms and environments.
  • Serverless edge deployment: Making the client compatible with serverless edge environments empowers users to harness the advantages of ClickHouse for real-time analytics and data processing in these setups.
  • Flexibility: Introducing a pluggable interface for the underlying communication layer will allow users to customize and adapt the client to meet their specific requirements.

Additional Context:

Serverless edge computing has gained significant popularity due to its scalability and reduced infrastructure management overhead. By enabling the usage of @clickhouse/client in these environments, we can provide developers with a powerful tool for processing and analyzing data close to the edge, ultimately improving performance and reducing latency.

I sincerely appreciate your consideration of this proposal. I look forward to engaging in a fruitful discussion regarding the possibilities of making @clickhouse/client more platform-agnostic. Thank you for your attention to this matter, and I kindly request your support in addressing this limitation.

@andrezimpel
Copy link

Been running into the same issue right now, using ClickHouse for Analytics with Vercel/Next.js edge functions.

@slvrtrn slvrtrn added the enhancement New feature or request label May 25, 2023
@slvrtrn
Copy link
Contributor

slvrtrn commented May 25, 2023

This is similar to #153 and somewhat related to #150

Currently, we are considering several scenarios to move forward with the client, preferably with as little breaking change as possible, for example:

  1. Replace the connection layer with a one-size-fits-all implementation (suitable for Node.js and Browser/etc with polyfills) that at least will not use a built-in Node.js HTTP module. That implementation could be based on, for example, fetch or axios. But, for instance, node-fetch has its own problems on the Node.js platform ("socket hang up" / ECONNRESET on consecutive requests with Node.js 19 and Node.js 20 node-fetch/node-fetch#1735, Error "SyntaxError: Cannot use import statement outside a module" when using node-fetch in Jest with Typescript node-fetch/node-fetch#1588, etc). However, it's unclear (or, more precisely, I haven't investigated it yet) how to support features such as mutual TLS in this scenario, as it uses https.Agent currently.

  2. Extract the current Node.js-specific connection layer into a separate package (and call it something like @clickhouse/client-node-connection) and require the Connection class to be passed when the client instance is created. This way, it will be possible for the users to implement a custom Connection that could work in their runtime.

^ All of the above covers usage of the http and zlib modules. As for the streams module, I think we could adjust it to use https://www.npmjs.com/package/web-streams-polyfill in the client based on the availability in the current runtime.

Personally, I am more in favor of the second option with a customizable Connection.

Please let me know what you think.

@maxprilutskiy
Copy link
Author

maxprilutskiy commented May 26, 2023

@slvrtrn, thanks for replying so quickly!

The second solution you described is a good middle ground. I like it because it opens up the original library and allows for customization of its network behavior.

However, since you will have to update the code in that specific place (meaning you'll have to rewrite how that layer of the library currently works), why not update it to use fetch APIs by default? You could still provide the option for customization through @clickhouse/client-<whatever>-connection if needed.

Correct me if I'm wrong, but the references you linked are related to node-fetch, while Node.js has a built-in native fetch implementation since version 18.x.

In your opinion, would that be a good solution?

By doing so, you would be using the fetch API for communicating over HTTP, which is the de facto new standard and is supported in 100% of modern environments: Node.js, Cloudflare Edge Workers, Deno.js, and, God forbid, even client-side browsers.

@slvrtrn
Copy link
Contributor

slvrtrn commented May 30, 2023

@maxprilutskiy, fetch is experimental in Node.js, as well as WebStreams support. Additionally, fetch is 18.x+ only, while we still want to support 16.x as it is not EOL yet. I did some experiments lately with customizable connections; however, after a brief internal discussion, we are leaning towards just releasing two libraries - @clickhouse/client (Node.js as it is now) and @clickhouse/client-browser (with "normal" fetch, for everything else), sharing some common types such as settings via workspaces in the codebase.

@maxprilutskiy
Copy link
Author

@slvrtrn awesome news!! Any approximate estimates of when this will become available?

Also a side note that it's not only about http, but also other modules like stream and zlib that fail (at least) on cloudflare

@slvrtrn
Copy link
Contributor

slvrtrn commented May 30, 2023

@maxprilutskiy, yes, stream, zlib, http etc will be gone from the "browser" version. The Node.js version will remain as-is. Buffer will probably stay in both but with a polyfill - again, I need to test it properly.

@maxprilutskiy
Copy link
Author

Sounds great!

@olexiyb
Copy link
Contributor

olexiyb commented May 30, 2023

Try to see how this package uses fetch
https://www.npmjs.com/package/fetch-undici
Undici actually build in since node 18.x, but you still can use it for node 16.x

@slvrtrn
Copy link
Contributor

slvrtrn commented Jul 19, 2023

The first web release is out: https://www.npmjs.com/package/@clickhouse/client-web.
See release notes: https://github.com/ClickHouse/clickhouse-js/releases/tag/0.2.0

I checked: it also works fine with Cloudflare workers, as it seems.

wrangler init my-worker -y

package.json:

{
  "name": "my-worker",
  "version": "0.0.0",
  "private": true,
  "scripts": {
    "deploy": "wrangler deploy",
    "start": "wrangler dev"
  },
  "devDependencies": {
    "@clickhouse/client-web": "0.2.0",
    "@cloudflare/workers-types": "^4.20230419.0",
    "typescript": "^5.0.4",
    "wrangler": "^3.0.0"
  }
}

worker.ts:

export default {
  async fetch(request: Request, env: Env, ctx: ExecutionContext): Promise<Response> {
    const client = createClient();
    const res = await client.query({
      query: 'SELECT * FROM system.numbers LIMIT 5',
    });
    return new Response(await res.text());
  },
};

prints:

{
	"meta":
	[
		{
			"name": "number",
			"type": "UInt64"
		}
	],

	"data":
	[
		{
			"number": "0"
		},
		{
			"number": "1"
		},
		{
			"number": "2"
		},
		{
			"number": "3"
		},
		{
			"number": "4"
		}
	],

	"rows": 5,

	"rows_before_limit_at_least": 5,

	"statistics":
	{
		"elapsed": 0.000438058,
		"rows_read": 5,
		"bytes_read": 40
	}
}

@maxprilutskiy @andrezimpel
can you please check if @clickhouse/client-web works for your use cases?

@slvrtrn
Copy link
Contributor

slvrtrn commented Jul 31, 2023

I am closing this as we have @clickhouse/client-web (without Node.js dependencies) released.
Please don't hesitate to create new issues in case of any findings.

@slvrtrn slvrtrn closed this as completed Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants