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: support overlay display unhandled runtime errors #2310

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nanianlisao
Copy link
Contributor

Summary

support overlay display unhandled runtime errors。

The rendering is as follows:
image

Related Links

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link

netlify bot commented May 11, 2024

Deploy Preview for rsbuild ready!

Name Link
🔨 Latest commit 5a2f9ac
🔍 Latest deploy log https://app.netlify.com/sites/rsbuild/deploys/664aeab8d29b120008d8a827
😎 Deploy Preview https://deploy-preview-2310--rsbuild.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 84 (🟢 up 8 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -0,0 +1,86 @@
// @ts-expect-error
import { SourceMapConsumer } from 'source-map';
Copy link
Member

Choose a reason for hiding this comment

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

I think import source-map will resolve to the Node.js version and get an error like this:

image

Copy link
Member

Choose a reason for hiding this comment

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

And we do not want to add third-party dependencies to @rsbuild/core, unless it can be prebundled to the compiled folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried to upgrade source-map to 0.7.4 and ran into some problems. It seems to be a problem with the source-map package itself.

Do you think it would work if I prebundled [email protected] into the compiled directory? If yes, I'll try to do it this way

Copy link
Member

Choose a reason for hiding this comment

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

No, [email protected] was released 7 years ago and we won't use a legacy version..

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, version 0.7.4 has a major problem that makes it unusable on the browser side. I don't think 0.5.7 should be rejected just because it's outdated, but whether it's more important to consider its usability.

I have found no good solution to this problem without changing the version. mozilla/source-map#459

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use source-map-js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can use source-map-js.

@9aoy Thanks for sharing.

I have a new problem. When I prebundler, I get an error: __filename is not defined.

I didn't find a good solution and had to improvise a less elegant solution.

The esm polyfill for __dirname is mostly Node-based. Sincerely hope to find a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

The prebundle is used to bundle Node.js packages, and source-map-js in imported in the client code, so it can not be prebundled.

Copy link
Member

@chenjiahan chenjiahan May 17, 2024

Choose a reason for hiding this comment

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

For the client code, I think the new code added in the PR should be imported on demand.

In other words, when the user does not enable the runtime error overlay, these code and source-map-js should not be bundled into the client code, otherwise it will introduce unused code and slow down the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, when the user does not enable the runtime error overlay, these code and source-map-js should not be bundled into the client code, otherwise it will introduce unused code and slow down the build.

Okay, let's take a look

@nanianlisao nanianlisao requested review from chenjiahan and 9aoy May 17, 2024 08:57
@nanianlisao nanianlisao force-pushed the feat_runtime_error_overlay branch from 82b06ba to ab644c0 Compare May 20, 2024 06:08
@nanianlisao nanianlisao force-pushed the feat_runtime_error_overlay branch from ab644c0 to 5a2f9ac Compare May 20, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants