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

Refactor use async control flow #3053

Merged
merged 9 commits into from Aug 15, 2019

Conversation

KSXGitHub
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Convert a chain of .then() into a control flow that uses await

@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #3053 into master will increase coverage by <.01%.
The diff coverage is 92.75%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/rollup/index.ts 93.68% <92.75%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5979144...02b222c. Read the comment docs.

@shellscape
Copy link
Contributor

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.

@KSXGitHub
Copy link
Contributor Author

why it's better than the code that previously existed.

  • A straightforward control flow is easier to reason about.
  • A chain of .then increases indent level.

Copy link
Contributor

@shellscape shellscape left a 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.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

See my comment

);
}
)
.then(
Copy link
Member

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.

Copy link
Contributor Author

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;
			}
		);

Copy link
Member

Choose a reason for hiding this comment

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

Exactly 👍

Copy link
Contributor Author

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).

@lukastaegert
Copy link
Member

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.

@KSXGitHub
Copy link
Contributor Author

KSXGitHub commented Aug 13, 2019

4aaf416 was my final commit.

Also it would be nice to resolve the conflict with master.

I must know which commits caused this conflicts.

Copy link
Member

@lukastaegert lukastaegert left a 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.

@shellscape shellscape self-requested a review August 14, 2019 11:35
@lukastaegert lukastaegert merged commit f0b429c into rollup:master Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants