-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(gatsby-source-filesystem): createRemoteFileNode rejects promise instead resolving on failure #12348
Conversation
@DSchau could you review this please? |
This generally seems very sensible (we really should do that from the start), but it does change behaviour: consumers of |
@DSchau - Is there a possibility to get this PR merged soon? |
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 |
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. |
Totally agree. Caught us out big time. Would be awesome to see this merged. |
discussing this with the team if we will bring this one in or not and break things. |
Thanks @wardpeet - There is always an option to not make it a breaking change, an optional parameter could be passed in ( |
Yeah, let's merge this 👍 |
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.
Let's ship this!
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:
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! |
|
Thanks @wardpeet and @KyleAMathews |
I'm having an |
Is it possible to share a code snippet? |
@wardpeet here you go! |
yes please do :) |
…nstead resolving on failure (gatsbyjs#12348)
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 withundefined
and outputting the error usingconsole.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