-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove try/catch from solid component check #3282
Conversation
|
The reason for the try/catch is to work when using multiple JSX frameworks in the same project. If you look at the React integration it does the same thing, but tries to figure out if the error is coming from somewhere else: https://github.com/withastro/astro/blob/main/packages/integrations/react/server.js#L37-L39 Can we do something similar here instead of removing the try/catch altogether? |
I'm not sure of any other way to determine if it's a Solid component. Maybe there's a way to disqualify React and Preact components though. Do you think it would be enough to just run through the checks in the order the integrations appear in the config? That way if React appears first in the config it would always pick that first unless the React check fails. It would also help to have a directive to manually specify component framework ie |
What if we did a try/catch when running .check(). https://github.com/withastro/astro/blob/main/packages/astro/src/runtime/server/index.ts#L204-L209 Save the errors that are throw. If all checks fail then we know the error was real, rethrow the error. Make sense? |
I think that would work. It's a bit odd from the perspective of the integration author though (at least for solid) because you would need to run |
@matthewp implemented the above, let me know if that works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@nonphoto need to resolve the new conflicts, but otherwise looks great! |
For the changeset would this apply to everything that depends on |
@nonphoto Sorry for the late reply. This only needs to apply to |
Going to merge to get the fix in and then add a changeset. |
Thanks! |
* Remove try/catch from solid component check * Move try/catch to renderComponent * Add solid to integrations-playground example
Fixes #3280
Changes
Remove try/catch from solid component check function. Solid components can be any function. Catching the errors in the check function prevents useful errors from being displayed in dev mode.
Testing
The test suite seems to be failing right now for "returns 404 for invalid URLs". Aside from that I'm not sure how to best test this as I'm not familiar with how this would affect other UI frameworks like React on the same site.
Docs
Ideally this shouldn't affect the docs.