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.

Sign up for GitHub

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

Jump to bottom

chore(gatsby): cleanup api runner promises #19664

Merged
merged 14 commits into from
Nov 26, 2019
Merged

chore(gatsby): cleanup api runner promises #19664

merged 14 commits into from
Nov 26, 2019

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Nov 20, 2019 •
edited
Loading

This attempts to remove a bunch of BlueBird promises from the api runner, in favor of native Promises.

It also cleans up the core of the api-runner by eliminating a bunch of controls that would never really be used due to how promises get resolved and non-blocking works.

@pvdz pvdz force-pushed the refactor-api-runner branch 2 times, most recently from c72cb4d to c2c1e0a Compare November 20, 2019 19:58
@vladar
Copy link
Contributor

vladar commented Nov 21, 2019

You could use async iterators with for await syntax unless I am missing something. They are available natively in node 10 but I guess babel/corejs have some polyfills for earlier versions.

@pvdz
Copy link
Contributor Author

pvdz commented Nov 21, 2019

It's an excellent point :)
Since we're targeting node 8 I'm going to hold off on that as I fear the Babel transform is going to be worse than what I have right now. Once we're on 10+ we can switch over and it would be a small change.

@pvdz
Copy link
Contributor Author

pvdz commented Nov 21, 2019

Had to test a few cases and conclusion is that this is not a safe refactor.

https://jsfiddle.net/3vuk4yfj/

In short, the refactor would not allow you to resolve the promise regarless of recursive ("cascading") calls. And it would not wait for all apis of the same name to finish before resolving the outer promise, which is also breaking the current model. So I'll go back to before that.

@pvdz pvdz force-pushed the refactor-api-runner branch 2 times, most recently from 8cede86 to 16f89c5 Compare November 21, 2019 11:26
@pvdz pvdz marked this pull request as ready for review November 21, 2019 11:34
@pvdz pvdz requested a review from a team as a code owner November 21, 2019 11:34
@pvdz pvdz removed the status: WIP label Nov 21, 2019
wardpeet
wardpeet reviewed Nov 26, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

At first glace this looks 👌. I've added 2 comments, nothing major. I'll be testing this on a few sites.

pvdz added 14 commits November 26, 2019 16:00
@pvdz
Abstract the api id generator
1e9a12a
@pvdz
Abstract the runPlugin bits (diff -w)
0569d18
@pvdz
Make runAPI an async arrow
848aed3
@pvdz
Use native Promises where possible
8ce4ed5
@pvdz
Drop the else
e816fd8
@pvdz
Modernize the promisify pattern
d389f2b
@pvdz
Unindent promise
71df15d
@pvdz
Replace bluebird mapSeries call with plain for-loop that awaits
b4550b5
@pvdz
Replace the "apis running map" with a simple counter
25726bb
@pvdz
Drop the api run instance id stuff as it is not used anymore
5d88190
@pvdz
Unabstract the incorrectly named afterPlugin func
47c2f68 
The diff is misleading; the runPlugin func wasn't moved, instead the afterPlugin was inlined back as that just makes more sense now.
@pvdz
Destructure all usages of args
3794cc5
@pvdz
Only generate the plugin name if an error is actually thrown
7ba9a59
@pvdz
Restore error tracking
5b044eb 
Not sure whether removing this check makes things better or worse so I'd better not touch it for now.
@pvdz pvdz force-pushed the refactor-api-runner branch from 16f89c5 to 5b044eb Compare November 26, 2019 15:00
@pvdz
Copy link
Contributor Author

pvdz commented Nov 26, 2019

(rebased)

@wardpeet wardpeet merged commit c523e22 into master Nov 26, 2019
@wardpeet wardpeet deleted the refactor-api-runner branch November 26, 2019 16:17
pvdz added a commit that referenced this pull request Jan 24, 2020
@pvdz
chore(gatsby): revert #19664
16016a0
pvdz added a commit that referenced this pull request Jan 24, 2020
@pvdz
chore(gatsby): revert #19664
e4052dc
@pvdz pvdz mentioned this pull request Jan 24, 2020
Merged
gatsbybot pushed a commit that referenced this pull request Jan 24, 2020
@pvdz
fix(gatsby): revert #19664 fix duplicate runs in develop mode (#20836)
9419bb3
pieh pushed a commit to pieh/gatsby that referenced this pull request May 17, 2020
@pvdz @pieh
chore(gatsby): cleanup api runner promises (gatsbyjs#19664)
8291c1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieh pieh pieh left review comments

@wardpeet wardpeet wardpeet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pvdz @vladar @pieh @wardpeet