fix(gatsby-plugin-gatsby-cloud): emit file nodes after source updates #33548
Conversation
Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
gatsbot
bot
added
the
status: triage needed
| @@ -113,6 +113,7 @@ exports.onPostBuild = async ( | |||
| } | |||
|
|
|||
| await Promise.all([ | |||
| maybeFileNodesEmittedPromise, | |||
There was a problem hiding this comment.
It's here so we actually wait on it somewhere to make sure we don't "finish the build" before ipc messages were actually sent.
There was a problem hiding this comment.
Question here, couldn't this have a race condition? What if the promise your listening to isn't the last one? Shouldn't we ad a global promise over the for each? that resolves when the whole foreach is done?
There was a problem hiding this comment.
This part need to change. This is not promise anymore (since we do maybeFileNodesEmittedPromise = await X below.
I think instead we should create single promise in API_FINISHED handler and only resolve it after each file was emitted (and awaited on) - then this check that should make sure that we don't exit before this is done would be actually correct?
| exports.onPreBootstrap = ({ emitter, getNodesByType }) => { | ||
| emitter.on(`API_FINISHED`, async action => { |
There was a problem hiding this comment.
onPreBuild in incremental updates is triggered before data update is applied, so that isn't an option (changing incremental updates so onPreBuild happen after data update is ... not straightforward, due to constant polling wether it should sourceNodes again we never know when would be "finalsourceNodes to triggeronPreBuild` then).
onPostBuild, while it would work, would be way too late - we do want to start file related work ASAP.
Hence adding this hacky "After sourceNodes" lifecycle (maybe it should be actual life cycle, I didn't want to create new one just for this purpose right now).
| maybeFileNodesEmittedPromise = await emitFileNodes({ | ||
| path: file.absolutePath, | ||
| }) |
There was a problem hiding this comment.
Shouldn't we remove the await here?
There was a problem hiding this comment.
await here is necessary for IPC backpressure. emitFileNodes will return true in most cases (in this case await will be a no-op) or promise when Node reports us that IPC needs back-pressure.
There was a problem hiding this comment.
okay, thought the queue was done all by emitFileNodes
There was a problem hiding this comment.
A queue is done by the Node itself, it lets us know when it is ready to resume sending IPCs via the callback (which we translate to a promise under the hood).
vladar
removed
the
status: triage needed
…#33548) * fix(gatsby-plugin-gatsby-cloud): emit file nodes after source updates Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com> * promise awaited on in onPostBuild should resolve after all file nodes were emitted Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
…gatsbyjs#33548) * fix(gatsby-plugin-gatsby-cloud): emit file nodes after source updates Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com> * promise awaited on in onPostBuild should resolve after all file nodes were emitted Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
…#33548) * fix(gatsby-plugin-gatsby-cloud): emit file nodes after source updates Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com> * promise awaited on in onPostBuild should resolve after all file nodes were emitted Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
Description
This moves emitting file nodes earlier and also fixes incremental builds not emitting updates.
Related Issues
[ch39979]