cross-document error propagation #755
Conversation
turbocrime
force-pushed
the
turbocrime/751-error-prop
branch
from
98d64d7 to
85a0110
Compare
There was a problem hiding this comment.
Hey, this seems to introduce regressions from #742 (which was merged earlier today). Can you please make sure those 3 cases are working first?
|
okay i got confused by the broken HMR :[ i thought i wasn't seeing the issue, because it kinda works depending on where you edit things wip pushed up i'll work on it tomorrow. annoyingly i think errors are propagating but they're getting detyped when they cross the dom message channel |
turbocrime
force-pushed
the
turbocrime/751-error-prop
branch
from
eaf1024 to
67d6b95
Compare
apps/extension/src/message/popup.ts
Outdated
There was a problem hiding this comment.
popup responses may be absent
There was a problem hiding this comment.
offscreen now uses ConnectError to serialize failures, and handles more failures
There was a problem hiding this comment.
popup utility catches a closed window into an absent response
There was a problem hiding this comment.
slice logic no longer necessary
apps/minifront/src/state/helpers.ts
Outdated
There was a problem hiding this comment.
we don't have ConnectError in this scope, so still doing string detection. i think if we want to detect actual codes instead of the code annotated in the message, that should be a separate PR
There was a problem hiding this comment.
maybe unnecessary to change, but racing every build result with cancel was excessive and caused more throws than necessary
There was a problem hiding this comment.
now using ConnectError in content scripts
turbocrime
force-pushed
the
turbocrime/751-error-prop
branch
from
7eb71bf to
0cc0314
Compare
turbocrime
changed the title
There was a problem hiding this comment.
Hey so, again, there's some very specific niche knowledge in here about messaging that makes it impossible to really review this without a more thorough PR description or inline comments. I added a few questions inline — if you can explain those, then I can give it a final review. But please do this before requesting a review in the future, because otherwise I have to do multiple rounds of reviews.
| @@ -52,18 +52,19 @@ export const approveOrigin = async ({ | |||
|
|
|||
| if ('error' in res) | |||
| throw errorFromJson(res.error as JsonValue, undefined, ConnectError.from(res)); | |||
| else if (res.data != null) { | |||
There was a problem hiding this comment.
Two things I think we should try to avoid:
- The equality operators that do casting (aka
==and!=). They are largely considered to be bad aspects of the javascript. It has the chance to not feel so logical. For example:'0' == false. This will returntrue. For these, we should always stick to the strict equals version===and!==. Null. Many folks consider this also to be a mistake in the javascript language given there are two primitives that basically represent that kind of state. Think we should always default toundefinedwhere we can. Though, I know in some places (like loaders), they require nulls.
should now convey serialized errors all the way to the client in every situation
no size diff
should help with #751 but there will be another underlying cause