Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow to pass errorHandler as record option #1107

Merged
merged 3 commits into from
Mar 22, 2023

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Feb 3, 2023

We've been playing around quite a lot with ways to handle errors that originate somewhere from within rrwebs deep callback stack. This PR adds an option to add a errorHandler option to record, which can be used to either swallow errors or handle them in different ways:

record({
  errorHandler: (error) => {
    // return `true` to swallow the error and stop propagating
    // Or log the error somewhere, etc.
  }
});

I tried to wrap all places that are called as (async) callbacks somewhere. I may have missed a place, but IMHO this should give a pretty decent coverage.

Note that in order to do that, I had to rewrite the monkey patching we are doing on e.g. CSSStyleSheet to use a proxy. Otherwise, retaining the this context correctly turned out quite tricky. IMHO this is a nicer approach anyhow, and all supported browsers (> IE11) support proxies, so should be OK I think.

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is a good mechanism. Using "Proxy" constructor will make the browser compatibility worse. Otherwise, it's a good way to do patching things.
Could you please solve the merge conflicts and add a change log for this one?

@mydea mydea force-pushed the fn/errorHandler branch from a2d434a to 12978c0 Compare March 20, 2023 12:01
@changeset-bot
Copy link

changeset-bot bot commented Mar 20, 2023

🦋 Changeset detected

Latest commit: 4144e08

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
rrweb Minor
rrweb-snapshot Minor
rrdom Minor
rrdom-nodejs Minor
rrweb-player Minor
@rrweb/types Minor
@rrweb/web-extension Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mydea
Copy link
Contributor Author

mydea commented Mar 20, 2023

I resolved the merge commits and added a changeset!

@YunFeng0817
Copy link
Member

Sorry, one more thing before merging this. Could you also add the description of this config in the rrweb doc?

@mydea mydea force-pushed the fn/errorHandler branch from 62f408f to 4befd4b Compare March 20, 2023 12:31
@mydea
Copy link
Contributor Author

mydea commented Mar 20, 2023

Sorry, one more thing before merging this. Could you also add the description of this config in the rrweb doc?

Ah sure, of course! I added this to the guide doc as well.

@YunFeng0817
Copy link
Member

Thank you!
Seems like the CI keeps failing because of this lint error:
image

Could you help me to add this line to the ZH-CN doc:

| errorHandler             | -                  | 一个可以定制化处理错误的毁掉函数,它的参数是错误对象。如果 rrweb recorder 内部的某些内容抛出错误,则会调用该回调。                                                                    |

@YunFeng0817 YunFeng0817 merged commit a225d8e into rrweb-io:master Mar 22, 2023
@mydea
Copy link
Contributor Author

mydea commented Mar 22, 2023

Thank you! Seems like the CI keeps failing because of this lint error: image

Could you help me to add this line to the ZH-CN doc:

| errorHandler             | -                  | 一个可以定制化处理错误的毁掉函数,它的参数是错误对象。如果 rrweb recorder 内部的某些内容抛出错误,则会调用该回调。                                                                    |

Ah, is this now resolved already? Or anything else I should do?

@YunFeng0817
Copy link
Member

Yes, I resolved it in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants