Skip to content
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: future-proof functionality through Gatsby schema customization #30

Merged
merged 1 commit into from
Dec 7, 2019
Merged

fix: future-proof functionality through Gatsby schema customization #30

merged 1 commit into from
Dec 7, 2019

Conversation

wKovacs64
Copy link
Contributor

What

Update the node augmentation logic to fall in line with the direction Gatsby is heading (i.e. remove direct node manipulation) to prevent immediate and future breakage.

Closes #28

How

Use Gatsby schema customization (createResolvers) to augment the user-supplied node type by adding a field with a user-supplied name where the resolver for said field returns a File node created by createRemoteFileNode (from gatsby-source-filesystem).

Notes

💥 This bumps the required Gatsby peer dependency to 2.2.0 or greater, as that is when the new schema customization APIs were introduced. Not a huge deal, as it has been out since March of 2019, but it is a breaking change and (IMHO) warrants a v2.0.0 release of this plugin.

💡 I didn't see a slick way to communicate between the onCreateNode API and the createResolvers API, so I introduced a module-level variable to track the relationship between the node to be augmented and the corresponding dynamically created (remote) File node. It felt a little weird, but it works. One alternative could be a "hidden" (undocumented) plugin option as both API methods get those passed in, but that felt ever stranger.

🔧 As the new File nodes are no longer being directly injected into the source/parent nodes, the previously existing tests were no longer able to verify the correct File node was present. In some cases, I adapted them to just ensure things were being called with the appropriate parameters and in other cases I simply removed the node verification assertion. It's not ideal and I suspect they can be improved, but it's better than nothing. A follow-up PR or suggestions to improve them would be great.

BREAKING CHANGE: gatsby-plugin-remote-images now requires Gatsby v2.2.0 or later.
@wKovacs64
Copy link
Contributor Author

cc @vladar

@graysonhicks
Copy link
Owner

@wKovacs64 This is great, and I agree would work best as a new major release since it will be using a new underlying Gatsby API that is not available in pre 2.2.0 Gatsby. I will do a version 1 release with the prettier changes in #29 then a 2.0 with these changes.

I'll try and get this pulled down and test it locally and have a look the module level variable.

@smurrayatwork might have some thoughts on the tests.

Lastly, will this help address using the plugin with gatsby-source-graphql as mentioned in #7 ? I recall seeing some similar solutions in a blog somewhere lately.

@wKovacs64
Copy link
Contributor Author

@graysonhicks As it stands, this won't address the gatsby-source-graphql issue in #7. It could be adapted to do so (potentially) by moving the node creation into the resolver, but that probably means doing away with onCreateNode entirely. I suspect the imagePath array literal notation feature is going to make that much more obnoxious than it would be otherwise, but I'd need to give it some more thought.

P.S.
Probably no need to release a new 1.x if the prettier changes are the only difference as that's purely style and formatting. The end result won't be any different.

@graysonhicks
Copy link
Owner

graysonhicks commented Dec 7, 2019

This works for me locally when npm link'd to a test repo. Going to merge this in to develop and release w/ #29 for 2.0.0. 👍

@graysonhicks graysonhicks changed the base branch from master to develop December 7, 2019 02:55
@graysonhicks graysonhicks merged commit 8911ccc into graysonhicks:develop Dec 7, 2019
@wKovacs64 wKovacs64 deleted the schema-customization branch December 7, 2019 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants