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

Prevent throwing in react and solid component checks #11624

Merged
merged 5 commits into from
Aug 9, 2024
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 5, 2024

Changes

fix #11615

Svelte 5 is a function component, so it messes some of our existing renderers like React and Solid where their check() functions assumes JSX and wouldn't throw, but Svelte 5 trips them up because it's not JSX functions.

Anyways, I think it's more correct that these check functions do not throw since they only do checking, like how Preact works now.

Testing

No tests as we don't have them for Svelte 5, and it's a little tricky to emulate a component that would error 😬 Hopefully the current passing tests are enough though

EDIT: I have to tweak one test regarding runtime errors from Preact components, because previously it relied on the React renderer's check to throw the error if it failed to render, which I don't think is necessarily correct. For now, renderers with check() that simply runs the render will not be able to detect actual runtime errors to display on the error overlay.

Docs

n/a. bug fix.

Copy link

changeset-bot bot commented Aug 5, 2024

🦋 Changeset detected

Latest commit: f132ffd

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

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

@github-actions github-actions bot added pkg: react Related to React (scope) pkg: preact Related to Preact (scope) pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels Aug 5, 2024
Comment on lines 29 to -30
try {
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

I only refactored this part as the nested try isn't necessary. Before it's try { try..catch } finally {}. It can be flatten as try..catch..finally and work the same.

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 5, 2024
@Hugos68
Copy link

Hugos68 commented Aug 7, 2024

Hey, thanks for the quick response and fix! When can we expect to see this released?

@Princesseuh
Copy link
Member

For now, renderers with check() that simply runs the render will not be able to detect actual runtime errors to display on the error overlay.

How many frameworks does this affect? This seems like a pretty consequent drawback

@bluwy
Copy link
Member Author

bluwy commented Aug 8, 2024

They affect the react and solid integrations only. Preact already handles it correctly now. However, this only affects if you have more than one UI framework integration, e.g. if you have React and Svelte 4, you get this error with this PR:

Screenshot image

If you only have the React integration, Astro will fallback to it and shows the error correctly (same as before this PR):

Screenshot image

@Princesseuh
Copy link
Member

Oh okay, fine with me then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using bind: in Svelte 5 in conjunction with React causes error in MDX files.
3 participants