-
Notifications
You must be signed in to change notification settings - Fork 35
feat: added support for new gatsby image plugin. #120
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
feat: added support for new gatsby image plugin. #120
Conversation
… to contentstack image api
abhishek305
left a 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.
Looks good
| exports.resolveGatsbyImageData = async (image, options, context, info, { cache, reporter }) => { | ||
| if (!isImage(image)) return null; | ||
|
|
||
| const { generateImageData } = await import('gatsby-plugin-image'); |
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.
Is this imported inside of the function body because this dependency is required to be installed in Gatsby site code?
| "name": "Contentstack", | ||
| "url": "https://www.contentstack.com" | ||
| }, | ||
| "license": "MIT", |
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.
I think gatsby-plugin-image should be in peerDependencies if it is required to be installed with a site. That way it's easier to mark which versions of the image plugin are compatible with this source plugin.
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.
It might be worth throwing an error with a custom (and helpful) message about what went wrong if gatsby-plugin-image is not included in the site deps. One thing we've done in the past with errors like this is add a section to a source plugin README and then link to relevant section (on github) where we can get a little more verbose or add more complex instructions.
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.
If you think that many users will not need to install gatsby-plugin-image and you want to dynamic import the module inside of function calls, then a good place to log this warning would be in the gatsby node API onPreInit since this is the very first API that runs when bootstrapping.
| const { getGatsbyImageFieldConfig } = await import('gatsby-plugin-image/graphql-utils'); | ||
|
|
||
| const fieldConfig = getGatsbyImageFieldConfig( | ||
| async (...args) => resolveGatsbyImageData(...args, { cache, reporter }), |
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 a bit of a nit: Spreading the args here seems like it could be flaky in the case that additional args are ever passed to it. What about something like this instead since only two of those arguments seem to used in resolveGatsbyImageData anyways? (Obviously the function signature for resolveGatsbyImageData would have to change as well.)
async (image, options) => resolveGatsbyImageData({ image, options, cache, reporter }),|
Thanks a lot for your valuable feedbacks, @veryspry . |
| const getGatsbyImageData = async () => { | ||
| const { getGatsbyImageFieldConfig } = await import('gatsby-plugin-image/graphql-utils'); | ||
|
|
||
| const fieldConfig = getGatsbyImageFieldConfig( |
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 would be better to move into the schema customization life cycle with the createSchemaCustomization API since createSchemaCustomization only runs once during the gatsby build process (and also will not run during incremental builds) whereas setFieldsOnGraphQLNodeType runs once for each type on every build.
It makes sense to make the switch since the source plugin already uses this life cycle in Gatsby node. An example might look something like this:
const typeDefs = [
schema.buildObjectType({
name: "ContentStack_assets",
fields: {
gatsbyImageData: getGatsbyImageFieldConfig(...)
},
interfaces: ["Node"],
}),
]
createTypes(typeDefs);This code could live in the createSchemaCustomization function definition in gatsby-node around here
Added support for new gatsby image plugin.
Added support to crop, trim, pad and add background color to the image via options passed to resolver.