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

Async throw error delay #98

Open
chrisblossom opened this issue Jul 12, 2019 · 3 comments
Open

Async throw error delay #98

chrisblossom opened this issue Jul 12, 2019 · 3 comments

Comments

@chrisblossom
Copy link
Contributor

Would you accept a PR that would change how async errors are thrown so the error is thrown after all pending files being deleted are removed? This would help deal with side effects a lot cleaner, especially when testing.

Similar to #90.

@sindresorhus
Copy link
Owner

Would you accept a PR that would change how async errors are thrown so the error is thrown after all pending files being deleted are removed?

Are you asking for this to be default behavior or opt-in?

This would help deal with side effects a lot cleaner, especially when testing.

How would it help?


p-map now has a stopOnError option we could use for this: https://github.com/sindresorhus/p-map/releases/tag/v3.0.0

@chrisblossom
Copy link
Contributor Author

Are you asking for this to be default behavior or opt-in?

I personally would like it to be the default (I think it is the safest), but I'd be happy either way.

p-map now has a stopOnError option

That makes the change really easy / already available (once the package is updated) how the options are passed to p-map.

Do you have a library that aggregates sync errors using aggregate-error similar to how p-map's stopOnError works?

How would it help?

test('race condition', async () => {
	// assume we expect a to throw an error for whatever reason
	const a = path.resolve('a.js');
	const b = path.resolve('b.js');

	await expect(del([a, b])).rejects.toThrow();

	const bExists = fs.existsSync(b);

	// will intermittently pass
	expect(bExists).toEqual(false);
});

@sindresorhus
Copy link
Owner

I personally would like it to be the default (I think it is the safest), but I'd be happy either way.

Yeah, I like it. Let's try it without even an option at first and see if anyone complains. (Would be a major release, obviously).

Do you have a library that aggregates sync errors using aggregate-error similar to how p-map's stopOnError works?

Maybe we could add a utility method to aggregate-error to do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants