Skip to content

THRIFT-4987: Fix TBinaryProtocol support in XHRClient#2157

Closed
MAXruppert wants to merge 1 commit into
apache:masterfrom
MAXruppert:THRIFT-4987
Closed

THRIFT-4987: Fix TBinaryProtocol support in XHRClient#2157
MAXruppert wants to merge 1 commit into
apache:masterfrom
MAXruppert:THRIFT-4987

Conversation

@MAXruppert

Copy link
Copy Markdown

Client: nodejs

The XHRClient uses the XMLHttpRequest web api to do requests. To correctly interpret binary data, the responseType needs to be set to arraybuffer, and the response property should be used instead of responseText (response defaults to normal text when not specified). See here.

I'd like to add tests but am unsure what the best practice would be for this with regards to the project testing framework? It would involve bundling the nodejs lib into a browser for testing, or perhaps running a headless browser environment in nodejs? I'd be happy to add tests, but would greatly appreciate if someone could point me in the correct direction.

I'm succesfully using this fix with a C++ HTTP server using the binary protocol for my use case.


  • Did you create an Apache Jira ticket? (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.

@stale

stale Bot commented Jul 25, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the wontfix label Jul 25, 2020
@stale

stale Bot commented Aug 1, 2020

Copy link
Copy Markdown

This issue has been automatically closed due to inactivity. Thank you for your contributions.

@stale stale Bot closed this Aug 1, 2020
@emmenlau

emmenlau commented Aug 3, 2020

Copy link
Copy Markdown
Member

This seems like a valuable change. And while I'm not any expert in JS, it looks sufficiently small to raise at least some trust, i.e. it mostly adds new behavior for the binary use case that was not handled before. The critical change seems to be:
https://github.com/apache/thrift/pull/2157/files#diff-29759a12f6599c6aeca4c0ae88af5291R116

Could someone test it, or should me merge it on best trust and wait for user feedback?

@nithinkdb

Copy link
Copy Markdown

Any chance this change could be merged? My team has a use-case that should be done via XHR, but we are needlessly using WebSocket because of this bug.

@nithinkdb nithinkdb mentioned this pull request Aug 22, 2022
5 tasks
CJCombrink added a commit to CJCombrink/thrift that referenced this pull request Mar 12, 2026
- Picking up THRIFT-4987 based on feedback and other PRs and local thrift patches for npm package
- Changes most probably originated from apache#2645 and apache#2157 that went stale and PRs were closed
CJCombrink added a commit to CJCombrink/thrift that referenced this pull request Mar 13, 2026
- Picking up THRIFT-4987 based on feedback and other PRs and local thrift patches for npm package
- Changes most probably originated from apache#2645 and apache#2157 that went stale and PRs were closed
CJCombrink added a commit to CJCombrink/thrift that referenced this pull request Mar 17, 2026
- Picking up THRIFT-4987 based on feedback and other PRs and local thrift patches for npm package
- Changes most probably originated from apache#2645 and apache#2157 that went stale and PRs were closed
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.

3 participants