throw a new wrapped error instead of default one from segment#10118
throw a new wrapped error instead of default one from segment#10118
Conversation
4baffa6 to
e7fab4d
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Let's add a comment explaining why we're doing this, and the horrors we invite if we don't.
Also, should we try to preserve any other properties? Could we preserve the stack here? 🤔
|
Preserved the stack, which i feared would also blow it up, but its good. Added an explanatory comment and named the new error 'safeError' |
Builds ready [4218e30]
Page Load Metrics (520 ± 44 ms)
|
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM! I have tested this with the same repro I used to see v8.1.9 freeze, and this does seem to resolve the problem.
I tested this with a build made using the command SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='aaa' SEGMENT_LEGACY_WRITE_KEY='aaa' yarn dist. On Firefox, I tested with Enhanced Tracking Protection set to strict mode, which results in segment.io being blocked. On Chrome, I just blocked segment.io in the dev console for the background process. On both browsers, the UI would freeze during onboarding before this commit but not after.
I think there is still some room for improvement here - we can detect the specific errors causing this problem (the axios errors) because they have the property isAxiosError, and we can pull more useful information from them like the HTTP status code. But that can wait for another time.
@Gudahtt was able to reproduce the firefox freezing using tracking protection in Firefox, or by blocking segment in chrome. The problem, as he discovered, seems to be in something that segment was doing with its error objects. I just changed the metametrics implementation to reject with a new error, using the original error's message and stack trace