[gatsby-source-filesystem] First parse url and then path to retrieve extension #9011
Conversation
oorestisime
changed the title
We test extensions in |
| @@ -195,7 +196,7 @@ async function processRemoteNode({ | |||
|
|
|||
| // Create the temp and permanent file names for the url. | |||
| const digest = createHash(url) | |||
| const ext = path.parse(url).ext | |||
| const ext = path.parse(Url.parse(url).pathname).ext | |||
There was a problem hiding this comment.
This is looking good to me! The basic idea is that it's going to make parsing the extension a little more robust, which is great.
How about some tests validating the approach? Also just to sanity check this, I created a Runkit to check this out!
There was a problem hiding this comment.
let's move path.parse(Url.parse(url).pathname).ext to separate function, that we can export and unit test against couple of urls including https://scontent.xx.fbcdn.net/v/t51.2885-15/42078503_294439751160571_1602896118583132160_n.jpg?_nc_cat=101&oh=e30490a47409051c45dc3daacf616bc0&oe=5C5EA8EB
There was a problem hiding this comment.
@oorestisime this is actually what I was going to suggest. If we move to a separate file/function, we can add some unit tests sorta like what I've done in the Runkit!
There was a problem hiding this comment.
yes sounds perfect. added this with latest commit. thanks
|
|
||
| describe(`create remote file node`, () => { | ||
| it(`can correctly retrieve files extensions`, () => { | ||
| expect([ |
There was a problem hiding this comment.
Here's a cool way to write these tests :)
// note: you may need to remove the dot from extension
[
[`https://scontent.xx.fbcdn.net/v/t51.2885-15/42078503_294439751160571_1602896118583132160_n.jpg?_nc_cat=101&oh=e30490a47409051c45dc3daacf616bc0&oe=5C5EA8EB`, `.jpg`],
[`https://facebook.com/hello,_world_asdf12341234.jpeg?test=true&other_thing=also-true`, `.jpeg`],
[`https://test.com/asdf.png`, `.png`],
[`./path/to/relative/file.tiff`, `.tiff`],
[`/absolutely/this/will/work.bmp`, `.bmp`],
]
.forEach(([url, ext]) => {
expect(getRemoteFileExtension(url)).toBe(ext)
})There was a problem hiding this comment.
ty, didnt occur to me like that :)
|
I think this looks great - one NIT: please rename test file to |
|
Oups sorry! corrected |
* First parse url and then path to retrieve extension * Move out extension parsing to unittest * Prettify test * Rename test file to match source file
* First parse url and then path to retrieve extension * Move out extension parsing to unittest * Prettify test * Rename test file to match source file
I was working on a plugin that uses the createRemoteFileNode and my urls had the following form (comes from instagram api)
https://scontent.xx.fbcdn.net/v/t51.2885-15/42078503_294439751160571_1602896118583132160_n.jpg?_nc_cat=101&oh=e30490a47409051c45dc3daacf616bc0&oe=5C5EA8EBIt was creating the following file nodes
And the extension was basically broken.
This lead to weird bugs in gatsby image. I.e querying for localfile like:
was giving
Haven't really checked why with that extension this breaks (maybe something to do with
&), i can dig in more if you want, but with that fix everything works again perfectly (plus i think its normal to first parse the url and then the path)