perf(gatsby): test sync before calling onCreateNode

[pvdz]

This change allows a plugin to register a shouldOnCreateNode callback which is called when Gatsby is about to schedule a callback for the (async) onCreateNode callback. The scheduling is relatively expensive at scale and the test is often cheap. As a result, this saves roughly 20% of a certain step during bootstrap for some cases (lot's of conditions, but this will apply to most sites to a certain degree). In the site I was benchmarking it drops a minute from a five minute step.

The problem is particular to onCreateNode because it will call the callback for each plugin for every node generated, even though in many cases the plugin may not care about that particular node (sharp does not care about markdown nodes, callback is still scheduled and invoked async).

Turns out that these promises do add quite some back pressure at scale. Who woulda thought.

[KyleAMathews]

💜 this. Great idea and simple.

[wardpeet]

I've added some bikeshedding comments, happy to hear what you think.

[pvdz]

Set the version to .79 and also the snapshot that changed because of it. Tests should pass now. Will publish after.

[pvdz]

Added a pluginOptions arg to the callback because plugins like mdx will need that and it's going to be easier to make that change now than to do it later down the line.

[pieh]

Not a blocker - just to not miss this part:

skipping runAPI makes this not behave like rest of APIs (which is also kind of the point of it), but it changes few things (like right now passing node plainly as first arg as opposed to passing object with node like actual onCreateNode and also not passing rest of "shared" things - https://www.gatsbyjs.com/docs/node-api-helpers/).

I don't think we need all of those helpers in the "test" callback like that (it should be quick check and not rely on anything else other than node itself), but it does introduce first "odd" gatsby-node hook and so we also need to think about user expectations (and our docs)

[pvdz]

@pieh @wardpeet blocking or not? :)

[wardpeet]

🤔 I agree with @pieh so let's do it differently. Let's call it unstable_shouldOnCreateNode({ node: args.node }, plugin.pluginOptions) so we're unblocked and it shows that it's not a complete API.

[pvdz]

I like that. Will roll with that

[pvdz]

eslint does not approve. Will go with unstableShouldOnCreateNode instead, otherwise every mention of it will also require an eslint suppression line.

[pvdz]

plot twist: eslint has been taught a lesson instead

[pvdz]

I'm going to ignore those starters errors. They seem unrelated and nothing else indicates a failure for this PR.

[pvdz]

Published in [email protected]