fix(gatsby-source-filesystem): createRemoteFileNode rejects promise instead resolving on failure

[antoinerousseau]

Description

When createRemoteFileNode fails to download a remote file (e.g. 404), it returns a rejected Promise, with the error containing the failing URL and the error message, instead of resolving with undefined and outputting the error using console.log.
Additionally, it also returns a rejected Promise if the requested URL is invalid, instead of resolving with undefined.

This is a breaking change: we should inform users that they should catch possible rejections.

Related Issues

This PR fixes #12280

[antoinerousseau]

@DSchau could you review this please?

[pieh]

This generally seems very sensible (we really should do that from the start), but it does change behaviour: consumers of createRemoteFileNode will need to be changed from checking if function returned promise resolving to null/undefined to catching errors, so I would consider this a breaking change.

[leonfs]

@DSchau - Is there a possibility to get this PR merged soon?

[wardpeet]

it's probably best to wait for gatsby v3 to actually merge this one as it's a breaking change in a widely used package. Technically we could bump it's major to vesion 3 but unsure what @pieh thinks

[leonfs]

Thanks, @wardpeet - I dissent with your opinion about being a breaking change, I think it is a bug more than a change in the contract. The current implementation returns a promise, and it is expected that as such it can throw. Most packages I've seen using it have a try/catch around it, though the catch will never run because it always resolves.

We had a bug in our platform because we didn't get the failure, and the build continued as if nothing happened.

[roylines]

Totally agree. Caught us out big time. Would be awesome to see this merged.

[wardpeet]

discussing this with the team if we will bring this one in or not and break things.

[leonfs]

Thanks @wardpeet - There is always an option to not make it a breaking change, an optional parameter could be passed in (rejectOnError) defaulted to false. That way current consumers don't get the change in expected behaviours. Then on v3 make the breaking change and remove the optional parameter.

[KyleAMathews]

Yeah, let's merge this 👍

[wardpeet]

Let's ship this!

[gatsbot]

Holy buckets, @antoinerousseau — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

[wardpeet]

Successfully published:
 - gatsby-source-filesystem@2.0.35

[leonfs]

Thanks @wardpeet and @KyleAMathews

[antoinerousseau]

I'm having an UNHANDLED REJECTION when fetching URLs having a 404 error, even when I catch the createRemoteFileNode Promise... any clue?

[wardpeet]

Is it possible to share a code snippet?

[antoinerousseau]

@wardpeet here you go!
https://github.com/antoinerousseau/gatsby-bug-mvce
Should I open an issue maybe? Or did I miss something?

[wardpeet]

yes please do :)

[antoinerousseau]

@wardpeet opened #14178