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

Add support for fetch and cbor #797

Closed
wants to merge 1 commit into from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Dec 3, 2018

This does quite a few sweeping changes to support fetch, CBOR encoding and the Accept header which is part of matrix-org/matrix-spec-proposals#1740. Please do not merge or review this just yet

Anyway, this PR seems to work perfectly fine with a few issues noted below.

Notable failings:

  • supportsCbor is always true, and not detected. Ergo, this PR breaks support for regular JSON servers.
  • I had to swap request for fetch because I could not get ArrayBuffer responses out of it which is important.
  • borc is the library we are using for cbor and is wasmy, untested, and seems to work most of the time.

This change is marked as an internal change (Task), so will not be included in the changelog.

if (!headers['Accept']) {
headers['Accept'] = 'application/json';
if (this.supportsCbor) {
headers['Accept'] = `application/cbor, application/jsonq=0.9`;
Copy link
Member

Choose a reason for hiding this comment

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

missing semicolon before "q=0.9"

@bradjones1
Copy link

The related MSC hasn't gone anywhere for a few years but fetch support would be good to have. Might be advisable to break out the CBOR-related components to make this more atomic.

@germain-gg
Copy link
Contributor

It appears that native fetch support has been merged in Node.js quite recently, nodejs/node#41749

@bradjones1 bradjones1 mentioned this pull request Feb 9, 2022
@MadLittleMods MadLittleMods added the T-Task Tasks for the team like planning label Jun 2, 2022
@3nprob
Copy link
Contributor

3nprob commented Aug 9, 2022

It appears that native fetch support has been merged in Node.js quite recently, nodejs/node#41749

Noteworthy that the v18+ implementation is provided by undici, so using that as a polyfill for runtimes without fetch support should ensure minimal differences.

@t3chguy
Copy link
Member

t3chguy commented Aug 9, 2022

Closing this as it no longer applies cleanly

@t3chguy t3chguy closed this Aug 9, 2022
@3nprob
Copy link
Contributor

3nprob commented Aug 9, 2022

Is the current intention still to go with fetch APIs across the board (last update I can see on the matter) or is still still up for reconsideration?

@t3chguy
Copy link
Member

t3chguy commented Aug 9, 2022

Yes, fetch given native browser and node support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants