-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refactor use async control flow #3053
Refactor use async control flow #3053
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3053 +/- ##
==========================================
+ Coverage 88.73% 88.73% +<.01%
==========================================
Files 165 165
Lines 5733 5734 +1
Branches 1748 1748
==========================================
+ Hits 5087 5088 +1
Misses 388 388
Partials 258 258
Continue to review full report at Codecov.
|
Thanks for the PR 🍺 It's super important to explain the motivation for your change. Please add a description to the pull request to explain why you made the change, what benefits it will have moving forward, and why it's better than the code that previously existed. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't personally find this any easier to read than the code it proposes to replace, so I can't grant approval since this is a subjective preference change. Other maintainers: please feel free to disregard my review if you collectively disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment
src/rollup/index.ts
Outdated
); | ||
} | ||
) | ||
.then( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to a more async-await based flow is definitely something we want but this is not an improvement now that the different styles are mixed and we have async functions inside Promise chains inside an async function. If you complete the refactoring to use a try-catch
block to catch the error and get rid of the remaining Promise chains in this section, I could see this merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukastaegert You want this?
let chunks: Chunk[]
try {
await graph.pluginDriver.hookParallel('buildStart', [inputOptions]);
chunks = await graph.build(
inputOptions.input as string | string[] | Record<string, string>,
inputOptions.manualChunks,
inputOptions.inlineDynamicImports as boolean
);
} catch (err) {
await graph.pluginDriver.hookParallel('buildEnd', [err]);
throw err;
}
await graph.pluginDriver.hookParallel('buildEnd', []);
For reference, this is the original:
const chunks = await graph.pluginDriver
.hookParallel('buildStart', [inputOptions])
.then(() =>
graph.build(
inputOptions.input as string | string[] | Record<string, string>,
inputOptions.manualChunks,
inputOptions.inlineDynamicImports as boolean
)
)
.then(
async chunks => {
await graph.pluginDriver.hookParallel('buildEnd', []);
return chunks;
},
async err => {
await graph.pluginDriver.hookParallel('buildEnd', [err]);
throw err;
}
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I already done that (4fcacdf).
As I see you are doing further refactorings on this PR: Is this to be merged or do you expect to do further work on this? Also it would be nice to resolve the conflict with master. |
4aaf416 was my final commit.
I must know which commits caused this conflicts. |
…c-control-flow # Conflicts: # src/rollup/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I manually resolved the conflict with master. From my side this is good to be merged. @shellscape please have another look if you still have reservations, I think in the current form it makes the error flow clearer.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Convert a chain of
.then()
into a control flow that usesawait