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

Full implementation of xhr.responseType, rigorous test and a perf imp… #16

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

pringseth
Copy link

…rovement.
Implemented section 3.6, subsections 8,9,10,11 of https://xhr.spec.whatwg.org/#the-response-attribute.
The test: test-response-type.js excerises it well.
Basically async download looks like this:

    xhr.open("GET", url, true);

    xhr.responseType =  responseType;

    xhr.onloadend = () => {
      if (xhr.status >= 200 && xhr.status < 300)
      {
        switch (responseType)
        {
          case "":
          case "text":
            resolve(xhr.responseText);
            break;
          case "document":
            resolve(xhr.responseXML);
            break;
          default:
            resolve(xhr.response);
            break;
        }
      }
      else
      {
        const errorTxt = `${xhr.status}: ${xhr.statusText}`;
        reject(errorTxt);
      }
    };

    xhr.send();

In order to use xhr.responseType="document" the npm package "@xmldom/xmldom" needs to be installed.
There's also been a perf improvement (new test: test-perf.js ):

As part of the implementation, there has been a major perf improvement.
On a 3 year old i9-10900K, a local download using this changelist of a 400MB Float32Array 
from node 'http' module takes about 300ms. However, using xmlhttprequest-ssl v2.1.1 it takes 372734ms.
That's over 1200x faster.

paul added 3 commits October 9, 2023 15:25
…. When the default xhr.responseType we only set xhr.responseText. To get the desired xhr.response we need to separately GET with xhr.responseType='arraybuffer'.
@mjwwit
Copy link
Owner

mjwwit commented Oct 10, 2023

Thanks for this! Looking at the scope of the changes I think I'll need some time to review it.

tests/test-sync-response.js Outdated Show resolved Hide resolved
tests/test-response-type.js Outdated Show resolved Hide resolved
@YarnSaw
Copy link

YarnSaw commented Oct 24, 2024

Hey @mjwwit, would appreciate you re-reviewing this patch as well when you get a chance. I've gotten Paul to apply your feedback, and got added as a contributor to his fork & his blessing to take over applying any additional changes the branch may need. Thanks!

@mjwwit
Copy link
Owner

mjwwit commented Oct 25, 2024

This PR is huge, and it's been too long since I last looked at it. It will take me significantly longer to review than the previous 2.

@mjwwit
Copy link
Owner

mjwwit commented Oct 26, 2024

There's still lots of ES2015 code in here (const, let, Promise, etc.), which is incompatible with any Node.js version before 6. There's two things we can do here:

  1. either you spend a lot of time refactoring all of that (especially Promise code will be tough to refactor),
  2. I release this combined with a larger refactor that will pull this library into the current age.

The first option will require a possibly significant amount of work on your side, the second will delay the release of this code. I'm not yet sure how much time I'll need for the refactor, however.

As I'm not sure how high the need for this functionality is, I'll leave the decision to you.

@YarnSaw
Copy link

YarnSaw commented Oct 28, 2024

Hey! Sorry for delay, OS bricked and spent the weekend repairing it (there are two types of people in this world, those that have a backup of all their data and those that will. I've just made that transition...), fun times.

I would like this in, I'll get to refactoring when I get a moment, hope to get it up within a couple days from now for you.

Though after reading the XHR spec a lot over the last week this package is quite far behind, and I would be down to help out any way you'd like in updating the module in the future, let me know!

@YarnSaw
Copy link

YarnSaw commented Oct 31, 2024

Okay. I found that the oldest node version that currently worked was node 4, with earlier versions throwing on an Object.assign which was undefined before then. Updated all the tests here to work with node 4. Turns out thins like let/const are valid, so long as 'use strict'; is enabled!
Edit: well, what happened in CI? Time to fix that.

Copy link
Owner

@mjwwit mjwwit left a comment

Choose a reason for hiding this comment

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

I'll release this as a major update, to be on the safe side as I'm not entirely sure we're not breaking previous behavior.

@mjwwit mjwwit merged commit 70949f7 into mjwwit:master Nov 14, 2024
4 checks passed
@mjwwit
Copy link
Owner

mjwwit commented Nov 14, 2024

I couldn't get the keepalive test to run under Node.js 4 because of API changes, but even when changing the test to use Node.js 4 compatible API I couldn't get it to work. It's not the server, as I tried running that separately using Node.js 22 and it still wouldn't pass. None of this is really related to this PR other than that I've had to increase the minimum supported Node.js version to 12 (as this is the first version that passes the keepalive test). I hope this is no problem for you.

@YarnSaw
Copy link

YarnSaw commented Nov 14, 2024

That works for me, thanks!

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.

3 participants