mjwwit / node-XMLHttpRequest

XMLHttpRequest for node.js
http://thedanexperiment.com/2009/10/04/emulating-xmlhttprequest-in-node-js/
MIT License
15 stars 24 forks source link

Add support for Data URI #25

Open Bhpsngum opened 4 days ago

Bhpsngum commented 4 days ago

I've been working on a project using this dependency recently and the project usually requires working with fetching data from Data URIs.

For the current approach, I've been patching the original XMLHttpRequest.js file to add support for this type of URI, but this is not good for the long run.

So I have a question: Am I allowed to make a PR to add support for Data URI here? data-uri-to-buffer dependency might be needed to implement this, so I want to ask first if you want this package to be dependency-free.

mjwwit commented 3 days ago

Of course you're allowed to make PRs, it's open-source for a reason. I'd like the project to remain free of external dependencies though, so you'll need to have a very good reason to introduce one. I don't think transforming a data URI to a Buffer is very difficult right? You can of course get your inspiration from the data-uri-to-buffer project code. Looking forward to your PR!

Bhpsngum commented 2 days ago

Gotcha, I will try to create a new PR for this one, should be done very soon Also, I have another question: Normally, accessing local resources is forbidden in browsers (Browser returns "Not allowed to load local resources" when trying to fetch local files). And for web behaviour mimicking, should there be an option for users to enable the ability to load local files explicitly?

For example, instead of:

var XMLHttpRequest = require("XMLHttpRequest").XMLHttpRequest;
var xhr = new XMLHttpRequest();

We define it like

// preferred way will be like this
var xhrMaker = require("XMLHttpRequest");
var XMLHttpRequest = xhrMaker({
    filesystem: false, // whether to allow loading local files, default to `true`
});
var xhr = new XMLHttpRequest();

// for legacy support
// if user attempts to read `XMLHttpRequest`, it returns a constructor with default settings
var XMLHttpRequest2 = require("XMLHttpRequest").XMLHttpRequest;
var xhr2 = new XMLHttprequest2(); // this will have default behaviour, aka reading local files
mjwwit commented 2 days ago

For browsers this is done to prevent security risks. The same doesn't apply for Node.js. However, to emulate browser behavior, it would be nice to have a (non-standard) flag in the options as in your examples. I'd give it a more explanatory name like allowFileSystemSources or something similar. Loading a data: URI isn't using the file system though, right? The data is contained within the URI.

Bhpsngum commented 2 days ago

Yeah you are right, Data URI already contains data itself within the URI, so browsers are just parsing them. Also for property naming, allowFileSystemSources as you suggested is a good idea

Bhpsngum commented 2 days ago

After searching around code spaces, I've seen that ppl are also loading the library as const XMLHttpRequest = require("xmlhttprequest-ssl"), so the best way might be introducing a method, like .createInstance:

// proposal
var XMLHttpRequest = require("xmlhttprequest-ssl").createInstance({
    allowFileSystemSources: false, // whether to allow loading local files, default to `true`
});
var xhr = new XMLHttpRequest();

// for legacy support
var XMLHttpRequest2 = require("xmlhttprequest-ssl").XMLHttpRequest;
var XMLHttpRequest3 = require("xmlhttprequest-ssl");

// These are identical (ability to read local files)
var xhr2 = new XMLHttpRequest2();
var xhr3 = new XMLHttpRequest3();
mjwwit commented 2 days ago

The XMLHttpRequest constructor already has a non-standard option: agent. You could simply add it there.

Bhpsngum commented 2 days ago

XMLHttpRequest actually accepts no arguments per spec, so I guess the existence of opts in the parameters list is already non-standard and is supposed only for Node integration, so it should probably be fine if I add an option like opts.allowFileSystemSources to it right?

Also node:url library doesn't parse the Data URI well enough, and since this package's minimum node version is 12.0, can I change it to URL instead? Functionality remains the same as the old one, and new URL() also throws if it receives an invalid URL, so we can use it to early-throw in case the supplied URL is invalid.