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(gatsby): revert #19664 fix duplicate runs in develop mode #20836

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Jan 24, 2020

Reverts #19664 because it exposed a timing problem in promises ultimately leading to gatsby develop triggering a number of redundant rebuilds.

At the core to this is listening to the api queue drain event, API_RUNNING_QUEUE_EMPTY, which before would only fire once. After this change, that event fires three times for the same sequence of events (a file changing).

The reason seems to relate to the difference in how BlueBird resolves its promise vs doing it natively. In this case, changing the promise led to the queue draining before the next api call had a chance to fill it up again. And again. So each of these calls would now see an empty queue, fill it, completely run (in pseudo sync time), and drain the queue again. Each call would lead to announcing the start and drain of the api queue.

Over in gatsby develop we were listening for the drain event as a way of debouncing a refresh-in-progress.

We've triaged this and tried to come up with a better solution without having to do a revert but ultimately there's not a trivial way of getting what we want. We need to consider a proper state machine model, where api calls fall within a higher level task description, to be able to "wait" for a current update to finish. Right now such structure does not exist and we can't wait for anything because we can't know what's semantically going on within the gatsby machinery right now.

Resolved the merge conflicts together with @sidharthachatterjee . Despite the age of the commit, luckily this file doesn't see many updates, so it's mostly about keeping @LekoArts's changes. Which I think we've done.

@sidharthachatterjee sidharthachatterjee marked this pull request as ready for review January 24, 2020 13:14
@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner January 24, 2020 13:14
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Let's merge if tests pass

@@ -1,3 +1,4 @@
const Promise = require(`bluebird`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bluebird is still in package.json and yarn.lock

That's good

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jan 24, 2020
@pvdz pvdz changed the title chore(gatsby): revert #19664 chore(gatsby): revert #19664 which was triggering duplicate builds in develop Jan 24, 2020
@pvdz pvdz changed the title chore(gatsby): revert #19664 which was triggering duplicate builds in develop chore(gatsby): revert #19664 fix duplicate runs in develop mode Jan 24, 2020
@pvdz pvdz changed the title chore(gatsby): revert #19664 fix duplicate runs in develop mode fix(gatsby): revert #19664 fix duplicate runs in develop mode Jan 24, 2020
@gatsbybot gatsbybot merged commit 9419bb3 into master Jan 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the revert-19664 branch January 24, 2020 13:49
@sidharthachatterjee
Copy link
Contributor

Successfully published in [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants