-
Notifications
You must be signed in to change notification settings - Fork 26
Data URI support, redirect checks and more #26
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
Conversation
|
Thanks for this! Would you be willing to add some tests for this functionality as well? |
|
sure, give me a bit |
|
I added the tests, you can have a look around tho |
mjwwit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a lot of code here, which isn't surprising given the functionality that has been added. This is no problem, but it will take some time to review everything. To not keep you waiting I've reviewed it partially.
I apologize for the number of small comments. It's a messy project already and I'd rather not make it any worse by introducing (more) inconsistencies.
It's all fine, after all this project is quite old already, and considering the number of dependencies, need to be careful indeed. |
|
I'm very sorry, but my time is very limited at this moment. I hope I can find the time to finish the review this weekend. Please bear with me. |
|
it's all fine, just take your time |
mjwwit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor comments, overall this looks very good. Thanks a bunch!
I probably missed some usages of == or !=. Also, could you add your tests to the test script in package.json?
Co-authored-by: Michael de Wit <mjwwit@users.noreply.github.com>
Co-authored-by: Michael de Wit <mjwwit@users.noreply.github.com>
|
I've fixed the reviews, also due to the thing that we agreed to change from
|
|
oh wow, i guess i'll just write a nodejs script to automatically run the test case, the current one looks... not that nice to check. |
It may be a good time to introduce something like that, thanks. Eventually we'll probably need a decent test runner though. |
|
I might introduce some unit testing and code coverage frameworks like Istanbul + Mocha combination, but as for now, I'm just writing a simple node script. Might be useful in the future though. |
|
I'll release later today or tomorrow, thanks again! |
|
Thanks for the update as well 😄 |
|
Good idea, I'll make sure that's done before the release. |
(From issue #25)
Data URI protocol (
data:)' ','\t','\n','\v','\f','\r') in encoded + non-encoded form, and strict padding checksRedirects
new XMLHttpRequest({ maxRedirects: 20 /* default is 20 */ })Too many redirects, and attempting to redirect to any non-secure protocols (other thanhttpsandhttp) or invalid URI throws/dispatchesUnsafe redirecterror.Local file system
new XMLHttpRequest({ allowFileSystemSources: true /* default is true */ })Not allowed to load local resources: file://...error.Origin
new XMLHttpRequest({ origin: "https://example.com" /* default is no origin */ })xhr.openwill act as an absolute URL.Others
xhr.responseURLto reflect XMLHttpRequest specxhr.statusTextnow returns the correct status text, taken fromHttpIncomingMessage.statusMessagexhr.dispatchEventnow dispatches additional object to event parameter{ type: "event_type" }, e.g{ type: "loadend" }xhr.getResponseHeader()andxhr.getAllResponseHeaders()throwing errors when no responses or headers are present (for local resources)Codespace
Url.parse()to new WHATWG-compatiblenew URL()with more safe restrictions