Finish docgen plugin #928
Finish docgen plugin #928
Conversation
| @@ -0,0 +1,126 @@ | |||
| { | |||
There was a problem hiding this comment.
:-)
Could we ignore it like we do yarn.lock?
There was a problem hiding this comment.
Ya I think so. I'll remove it anyway.
| @@ -36,6 +36,7 @@ module.exports = async function onNodeCreate({ | |||
| content: data.content, | |||
| } | |||
| markdownNode.frontmatter = { | |||
| title: ``, // always include a title | |||
There was a problem hiding this comment.
meh, you get errors otherwise :/
|
Deploy preview failed. Built with commit b1a6d0a https://app.netlify.com/sites/using-drupal/deploys/5913589f8ebdd970d0abb531 |
|
Deploy preview failed. Built with commit b1a6d0a https://app.netlify.com/sites/gatsbyjs/deploys/5913a6dd424ef2062e402a3a |
|
Deploy preview failed. Built with commit b1a6d0a https://app.netlify.com/sites/gatsbygram/deploys/5913589f8ebdd970d0abb52f |
| }, | ||
| composes: { | ||
| type: new GraphQLList(GraphQLString), | ||
| resolve: resolve(`composes`), |
There was a problem hiding this comment.
Curious why you're doing this here vs. letting Gatsby handle creating the schema for them?
There was a problem hiding this comment.
Generally I think you should only explicitly create GraphQL schema bits when a) transforming the data is pretty expensive as with markdown or b) you want to give the user control over the transformation of the data e.g. image transformation.
There was a problem hiding this comment.
Originally I couldn't because of that bug in the input generation... Also I was worried that they wouldn't always be there depending on the sites components. Other than that tho I was just being explicit since I know the shape.
Honestly for the simpler ones here there isn't any good reason it's this way :P
There was a problem hiding this comment.
Unrelated, I noticed a bug in the inputs the arguments are getting created for description___NODE not description
There was a problem hiding this comment.
cool :-) a bug in input generation is a good reason!
You want to try removing this stuff real quick to see if everything works? Really want transformation/sourcing to be just a "throw data at Gatsby and you're done" type experience.
There was a problem hiding this comment.
May not be near a computer for a while (heading out for the weekend), happy to change it tho. But if you wanna get it out feel free to jump in, or merge and we can change it later
There was a problem hiding this comment.
I'll be mostly gone for the weekend too! So if you could jump on it Monday that'd be great. Have some paid work I need to do early next week.
|
This looks so cool! Looking forward to trying it out. |
|
think I'm done here 👍 |
| // TODO: if you want to support infering Union types this should be handled | ||
| // differently. Maybe merge all like types into examples for each type? | ||
| // e.g. union: [1, { foo: true }, ['brown']] -> Union Int|Object|List | ||
| if (!isSameType(obj, next)) return null |
There was a problem hiding this comment.
Right now it bails on this branch if the values are polymorphic
|
why is it it failing! |
| // TODO: if you want to support infering Union types this should be handled | ||
| // differently. Maybe merge all like types into examples for each type? | ||
| // e.g. union: [1, { foo: true }, ['brown']] -> Union Int|Object|List | ||
| if (!isSameType(obj, next)) return INVALID_VALUE |
There was a problem hiding this comment.
I don't know if this is better. The other option is if the types are different just use the first one we saw.
There was a problem hiding this comment.
I don't think union types are what we want. They're a lot less discoverable IMO. I think we should just want and say, "hey data provider! Group your different types under their own keys or node types."
There was a problem hiding this comment.
would you prefer to throw here then?
There was a problem hiding this comment.
Yeah let's. The vast majority of Gatsby users will never see it think about these issues. They'll just add source and transformer plugins and get going. So it's important that for the few creating plugins that we insist they get things right so little mistakes made there don't reverberate across everyone downstream.
There was a problem hiding this comment.
actually as I'm thinking about it, its tough to throw...A plugin author would most likely "fix" this during the extendNodes phase which is after example gathering and not accessible for input inference.
In the specific case here the union type is component.props[].type.value You could "fix" that in the node creation bit, but actually I just want to make that JSON scalar rather then split that out into 5 different fields, arrayValue, enumValue, shapeValue, customValue, etc. For one it differs fairly dramatically from the underlying source format and would require a bunch of documentation apart from react-docgen, I also think it's less ergonomic in this case
There was a problem hiding this comment.
Hmmm ok. Can definitely see that union types make sense here. No need to figure out everything right now though. After a few more dozen source and transformer plugins the patterns will be a lot more clear. A bit busy today but will try to merge this later along with getting my rr V4 PR in.
There was a problem hiding this comment.
we can always revisit, at the moment it's just not breaking on mixed types and leaving it to the plugin author to address. I'm not sure if stuff is still not building tho looking at the PR checks....stuff is running locally for me.
|
Oh... realized why the build is failing. I published a canary from my RR v4 branch to test something which means that's what your builds are building from so of course failing. I played loose and free with canary releases, etc. when I was the only one basically working on 1.0 but I need to be more careful with that now. |
|
It'd be nice too if the Netlify build logs were public... |
|
Thanks for the really nice PR! |
|
Oh actually I published the canary so my builds would succeed :-) Hmmm... this needs fixed somehow. |
ok mostly correct now. I haven’t completely tested it yet but should be 90% done at least :P