search

broccolijs / broccoli

Browser compilation library – an asset pipeline for applications that run in the browser
https://broccoli.build
MIT License
3.33k stars 217 forks source link

Async cleanup #443

Closed chriseppstein closed 4 years ago

chriseppstein commented 4 years ago

I'm on the hunt for rejection leaks in the broccoli code that cause the UnhandledRejectionWarning message to be displayed when there is a build error while running ember serve.

Since broccoli requires node 8, it can use async/await. So I've rewritten the builder's build method to use async/await instead of the complex promise chaining that it was doing before.

After I did this, one of the two unhandled rejection warnings I was seeing when I ran broccoli's tests appears to have gone away (I'm still seeing the warning in my app though).

I'm submitting this PR because I think the code is easier to read now.

stefanpenner commented 4 years ago

fixed one async issue: https://github.com/broccolijs/broccoli/pull/445

chriseppstein commented 4 years ago

To ensure we do not allow this test suite to pass if unhandledRejection occurs, could we run the following code, in-process prior to the tests running?

You pushed this change in #445.

stefanpenner commented 4 years ago

To ensure we do not allow this test suite to pass if unhandledRejection occurs, could we run the following code, in-process prior to the tests running?

You pushed this change in #445.

I actually need to re-think this idea, as the watcher holds promise, for a later request from the server to read. This can legitimately trigger the unhandled promise notification, although it isn't a real issue, in-fact it is "by design" only registered later and since the public API.

There are several approaches for us to take:

a. create a faux promise that doesn't trigger the unhandled rejection warning; this would allow us to hide the warning, without breaking public API b. have our watcher -> server promise, fulfill with a complex type{ value: unknown, type: 'FULFILLED', 'REJECTED' }; this allows the server full control, no warnings, but will also be a breaking change. b. do something totally different; I don't really thing that is needed here.

stefanpenner commented 4 years ago

Thank you for pointing out this code could be improved, the current approach makes the cancellation not work quite correctly, see: https://github.com/broccolijs/broccoli/pull/443#discussion_r385220733

closing in favor of a larger refactor: https://github.com/broccolijs/broccoli/pull/447 that moves to using async/functions where appropriate, while simplifying and commenting on the algorithms touched in this PR.