-
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-transformer-sharp): Warn when using unsupported extension #20782
Conversation
childImageSharp: { | ||
resolve: (parent, args, context, info) => { | ||
if (parent.extension === `gif`) { | ||
reporter.warn( |
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.
This has potential of spamming console. Do we need this warning for each instance of it?
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.
@pieh I guess we could also collect the paths (and counts) and print out only one warning with all paths at the end, e.g. with onPostBootstrap
or something?
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.
Printing it for every instance might be fine. I just wanted to raise potential problems it can cause.
I wouldn't recommend using onPostBootstrap
(as this will result in non-warning cases for gatsby develop (local / preview) as people change their data or queries in source code).
Ideally we would have "sessions" for each "query running" step where we dedupe warnings for same files within those, but I don't think plugins can do something like that right now.
I'll try to find "prior art" in other plugins for similar warnings and if we have any - let's follow what we already do in other plugins
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.
Maybe we should shorten the warning (to two lines)?
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.
@pieh shortened the message now
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.
This is great @LekoArts! I added 1 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.
Thanks! Looks great, our users will be very happy.
Published |
@LekoArts Is it supposed to warn for
Yet the image shows as expected so all those warnings are pretty annoying 😢 |
nevermind, the code says |
This caused a regression where
@CanRau this is probably why you see the warning for jpeg. |
Thanks for linking those 🙏 it's gone now 👍 |
Description
In the past people were (non surprisingly) confused when their image query e.g. from a CMS blew up and they couldn't figure out that it was cause by a .gif file they used.
Now a warning will be shown if a .gif file is queried and childImageSharp is used. I decided against an error and for a warning as people might guard themselves against that by checking childImageSharp for being
null
. It'll work then. Example query that shows an error:Output:
For now it doesn't specify the line + column as that would require a lot more additional work.
Documentation
Related Issues
Fixes #18699