Implement plugin and strategy changes proposal #2455
Conversation
| } | ||
|
|
||
| export interface HandlerWillStartCallback { | ||
| (param: HandlerWillStartCallbackParam): Promise<void | null | undefined>; |
There was a problem hiding this comment.
What's the significance to Promise<void | null | undefined> here?
I know the difference between null and undefined, but not sure how void comes into play. I'm also not sure why all three are possible values that the we want to support the Promise fulfilling with.
There was a problem hiding this comment.
It's tricky and I'm honestly not sure how best to solve this (I think it's a TypeScript bug/quirk).
void is recommended to only be used with functions (to denote no return value), but in the case of async functions that doesn't return a value, you have to use Promise<void>. However, as soon as you have any other code that stores the return value of an async function like this, you need to account for some functions explicitly returning undefined and some functions not having a return statement.
In the case of third-party plugin code, I think we need to support all three...
(Let me know if you have any thoughts on how to improve this.)
There was a problem hiding this comment.
In that case, I wonder whether letting folks explicitly return undefined or null is necessary vs. just requiring no return statement by using Promise<void>.
In other words, why do we want folks to be able to explicitly return undefined or null from their async callback function?
philipwalton
force-pushed
the
plugin-updates
branch
from
f6666a5
to
0aceebc
Compare
|
OK, I made a few updates based on your feedback, and I rebased against master to get the Node 12 changes. |
R: @jeffposnick
Fixes #2446
This PR implements the proposal outlined in #2446. It should not involve any breaking changes (in theory), but I'm merging it into the v6 branch just in case.
One thing to note here is that I've moved the
cacheWrapperandfetchWrappermodules fromworkbox-coreintoworkbox-precaching, and I've implemented them via a customStrategyHandlerinstance. While this is not ideal, it's only a temporary change since—onceworkbox-precachingis updated to use a custom strategy under the hood—thecacheWrapperandfetchWrappermodules can be deleted. For the moment, however, I wanted to move them out ofworkbox-corebecause I didn't wantworkbox-coreto have a dependency onworkbox-stategies.Another note is that I've made minimal changes to the existing tests, just to make sure I wasn't breaking or missing any of the existing behavior. Once we're comfortable merging this PR, we can go back and refactor the tests as needed.