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

Remove try/catch from solid component check #3282

Merged
merged 4 commits into from
May 11, 2022
Merged

Conversation

nonphoto
Copy link
Contributor

@nonphoto nonphoto commented May 3, 2022

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.

@changeset-bot
Copy link

changeset-bot bot commented May 3, 2022

⚠️ No Changeset found

Latest commit: 3c9af0c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels May 3, 2022
@matthewp
Copy link
Contributor

matthewp commented May 4, 2022

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?

@nonphoto
Copy link
Contributor Author

nonphoto commented May 4, 2022

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 integration:solid-js.

@matthewp
Copy link
Contributor

matthewp commented May 4, 2022

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?

@nonphoto
Copy link
Contributor Author

nonphoto commented May 4, 2022

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 createComponent just for the error side effects. I think anything that gives some feedback about what went wrong is better than the current option though.

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) labels May 6, 2022
@nonphoto
Copy link
Contributor Author

nonphoto commented May 6, 2022

@matthewp implemented the above, let me know if that works.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Looks great!

@matthewp
Copy link
Contributor

matthewp commented May 6, 2022

@nonphoto need to resolve the new conflicts, but otherwise looks great!

@github-actions github-actions bot added test 🚨 action Modifies GitHub Actions labels May 6, 2022
@github-actions github-actions bot removed 🚨 action Modifies GitHub Actions test labels May 6, 2022
@nonphoto
Copy link
Contributor Author

nonphoto commented May 6, 2022

For the changeset would this apply to everything that depends on astro? Not familiar with how monorepos work.

@nonphoto nonphoto requested a review from matthewp May 6, 2022 17:31
@matthewp
Copy link
Contributor

@nonphoto Sorry for the late reply. This only needs to apply to astro.

@matthewp
Copy link
Contributor

Going to merge to get the fix in and then add a changeset.

@matthewp matthewp merged commit abc5b21 into withastro:main May 11, 2022
@nonphoto
Copy link
Contributor Author

Thanks!

@nonphoto nonphoto deleted the fix-3280 branch May 11, 2022 19:06
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* Remove try/catch from solid component check

* Move try/catch to renderComponent

* Add solid to integrations-playground example
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: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Solid integration swallows component errors server-side
2 participants