Skip to content

THRIFT-5849: Expose createClient in browser version of nodejs package#3096

Merged
Jens-G merged 1 commit intoapache:masterfrom
graphcore:expose-createclient-browser
Feb 8, 2025
Merged

THRIFT-5849: Expose createClient in browser version of nodejs package#3096
Jens-G merged 1 commit intoapache:masterfrom
graphcore:expose-createclient-browser

Conversation

@cameron-martin
Copy link
Contributor

@cameron-martin cameron-martin commented Feb 5, 2025

Client: nodejs

createClient is exposed in a nodejs context, but not for browsers. Even though this is the same function as createWsClient, createHttpClient, etc, it would be better to be able to use the generic name with custom connection types. There is no reason I know of that prevents this from being exposed in a browser context - it appears to be more of an oversight.

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

`createClient` is exposed in a nodejs context, but not for browsers. Even though this is the same function as `createWsClient`, `createHttpClient`, etc, it seems odd to use these for custom connection types. Moreover, it is beneficial for the browser and nodejs interface to be as similar as possible.
@Jens-G Jens-G added the nodejs label Feb 5, 2025
@Jens-G Jens-G merged commit 0941aec into apache:master Feb 8, 2025
20 of 23 checks passed
@cameron-martin cameron-martin deleted the expose-createclient-browser branch February 10, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants