fix(preload): Fix error handling #6753
Conversation
Previously, preload errors were passed through the destroyPromise, which is contrary to our normal practices.
|
Actually, thinking about this some more, now that I have added |
lib/media/preload_manager.js
Outdated
| // Silence promise failures, in case nobody is catching this.successPromise_ | ||
| this.successPromise_.catch((error) => {}); |
There was a problem hiding this comment.
This is the only part I'm unsure of. It might mask important errors. Do we need this for our demo or tests? Or is it just preventative somehow?
There was a problem hiding this comment.
It's required for the tests.
I just tried this out to remind myself, and without this some of the tests fail. It complains about uncaught promise rejections.
There was a problem hiding this comment.
I think we should fix the tests, then, to catch any preload manager errors. Even if it's just silenced in the test case, that's better, I think, than silencing it globally in the library.
There was a problem hiding this comment.
I guess we might be able to change the tests instead? I'm not sure how many tests are broken because apparently an uncaught promise failure in afterAll causes the entire test run to stop, for whatever reason.
There was a problem hiding this comment.
I tested with:
./build/test.py --browsers Chrome \
--no-babel \
--uncompiled \
--delay-tests 2--delay-tests helped make errors after tests clearer by inserting a delay between each case. After narrowing it down to Player unit tests, I found out the problem was in start() and waitForFinish() returning two technically separate promises that resolve/reject on the same events/values. Clarifying that so that start() initiates the action only, makes it so that Player is catching errors in all the right places.
theodab
changed the title
|
Incremental code coverage: 100.00% |
|
@theodab please merge if you approve of the change I made |
|
Looks like it's failing a test... my bad |
|
I can't approve this since it's technically still my own PR, but I've looked over Joey's change and approve of it. I think we can now submit this, once it passes a full test run. |
joeyparrish
changed the title
After a previous bugfix to the preload system, we ended up with a situation where the overall progress in the preload was tracked by two promises: `successPromise_`, which is resolved when the preload finishes successfully. `destroyPromise_`, which is rejected with an error when the preload process trips an error condition. These two promises were confusingly named; it sounds like destroyPromise is related to the destroy process, but really it has more to do with errors. They were also completely redundant, as a single promise can be used to carry both a resolved and rejected state. This PR simply combines the two promises into one. --------- Co-authored-by: Joey Parrish <[email protected]>
After a previous bugfix to the preload system, we ended up with a situation where the overall progress in the preload was tracked by two promises: `successPromise_`, which is resolved when the preload finishes successfully. `destroyPromise_`, which is rejected with an error when the preload process trips an error condition. These two promises were confusingly named; it sounds like destroyPromise is related to the destroy process, but really it has more to do with errors. They were also completely redundant, as a single promise can be used to carry both a resolved and rejected state. This PR simply combines the two promises into one. --------- Co-authored-by: Joey Parrish <[email protected]>
After a previous bugfix to the preload system, we ended up with a situation where the overall progress in the preload was tracked by two promises: `successPromise_`, which is resolved when the preload finishes successfully. `destroyPromise_`, which is rejected with an error when the preload process trips an error condition. These two promises were confusingly named; it sounds like destroyPromise is related to the destroy process, but really it has more to do with errors. They were also completely redundant, as a single promise can be used to carry both a resolved and rejected state. This PR simply combines the two promises into one. --------- Co-authored-by: Joey Parrish <[email protected]>
After a previous bugfix to the preload system, we ended up with a situation where the
overall progress in the preload was tracked by two promises:
successPromise_, which is resolved when the preload finishes successfully.destroyPromise_, which is rejected with an error when the preload process trips an error condition.These two promises were confusingly named; it sounds like destroyPromise is related to the destroy process,
but really it has more to do with errors.
They were also completely redundant, as a single promise can be used to carry both a resolved and
rejected state.
This PR simply combines the two promises into one.