Fix Data URI handling and drop all URL analysis RegExps #853
Conversation
tinovyatkin
changed the title
tinovyatkin
requested review from
xxczaki,
bitinn,
jimmywarting,
LinusU and
Richienb
tinovyatkin
added
bug
enhancement
dependencies
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
#311 is an old issue worth bringing up, @TimothyGu first recommended that we used data-uri-to-buffer but later suggested that we also would use data-urls but the pr got closed by @Richienb who used native node functions (#659). |
|
yeah, my first intention was to use native |
|
well, #659 should never have been accepted. but that is gone now |
| return new Promise((resolve, reject) => { | ||
| // Build request object | ||
| const request = new Request(url, options_); | ||
| const options = getNodeRequestOptions(request); | ||
| if (!supportedSchemas.has(options.protocol)) { | ||
| throw new TypeError(`node-fetch cannot load ${url}. URL scheme "${options.protocol.replace(/:$/, '')}" is not supported.`); |
There was a problem hiding this comment.
Shouldn't this be a FetchError?
There was a problem hiding this comment.
All URL checking and unsupported protocol errors earlier were TypeErrors (like in browsers). Should we change?
There was a problem hiding this comment.
If this is a limitation of node-fetch then I think it should be a FetchError. Otherwise, a TypeError will do.
There was a problem hiding this comment.
No, it’s not node-fetch specific - original error is from Chrome in this case and says “Fetch API cannot load...” - I’ve just replaced fetch API with node-fetch
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Richienb
changed the title
test/main.js
Outdated
| @@ -2133,7 +2133,6 @@ describe('node-fetch', () => { | |||
| }); | |||
|
|
|||
| it('should accept data uri 2', async () => { | |||
| // this is from wikipedia: https://en.wikipedia.org/wiki/Data_URI_scheme | |||
There was a problem hiding this comment.
@Richienb why do you think this documentation comment is irrelevant? It's the source reference.
There was a problem hiding this comment.
A link to Wikipedia about data uris is not necessary since it might as well go in the readme.
There was a problem hiding this comment.
It's not only about data URIs, this specific URI for the test was copied from the article, so, any future maintainer will have no doubts that the actual URI is correct.
This PR adds a breaking test for existing dataUri implementation and reimplements it without using RegExp and also drops them in Request, following browsers logic on this case.
Only changes to tests are breaking case (from Wikipedia) for current implementation and adjusting of error messages as they are coming from native URL parser now (unsupported protocol message is copied from Chrome
fetch)