-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add preact/compat
support to @astrojs/preact
#3712
Conversation
Based on the current Preact renderer and the old preact/compat implementation: https://github.com/withastro/astro/blob/f892aeb52f5a93d81a68d9833eb26bedd06aa2f0/packages/renderers/renderer-preact/compat/index.js
🦋 Changeset detectedLatest commit: 85cf581 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 |
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.
Looking good!
I bet the issue with any errors like "react" not found
happens when any dependencies are treated as vite.ssr.external
—they'll use standard Node resolution rather than Vite's resolution. So in that case, the error is correct... react
cannot be found since it's not installed.
The fix is possibly to set vite.ssr.noExternal
to true
, ensuring that all dependencies are built.
No dice with
Next hint, please! 😅 |
@delucis I was able to get things mostly working by using manual aliases via |
Amazing work @natemoo-re!!! 🥳 This is very close to perfect. Here’s what works and doesn’t work now:
In Weirder is that the same behaviour happens on a page with just a Preact component if you visit it after having visited a page with a React component. It’s almost like that 504 Error crashes some internal or causes Vite to cache some bad code? Not sure. I’ll also stress — in case it’s important — that I’m testing this by copy-pasting the changes as a local integration in my test project. It’s on Stackblitz here: https://stackblitz.com/edit/github-6sydzv?file=integrations/preact.mjs |
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.
LGTM whenever this is ready for review! It'd be nice to add some docs about this to the README.
preact/compat
support to @astrojs/preact
Giving a README LGTM! |
Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com>
LGTM 🚀 |
Plenty of LGTMs, merge away @delucis! |
* Add preact/compat renderer (likely broken) Based on the current Preact renderer and the old preact/compat implementation: https://github.com/withastro/astro/blob/f892aeb52f5a93d81a68d9833eb26bedd06aa2f0/packages/renderers/renderer-preact/compat/index.js * Make sure name is consistent * Switch to single integration with compat option * fix: add module-resolver to alias react => preact/compat * fix: preact/compat mode * chore: remove client-compat entrypoint * chore: add e2e test for preact/compat * Try to fix frozen lock file error * Add changeset * Update README to new structure & document `compat` * Fix changeset wording * Fix README typo * Tweak wording Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com> Co-authored-by: Nate Moore <nate@astro.build> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com>
Changes
This branch tries to add a
compat
configuration option to the@astrojs/preact
integration based on the the oldpreact/compat
renderer.I don’t think this works yet — trying with a local copy of this integration on Stackblitz still produces a
Cannot find module 'react'
error: https://stackblitz.com/edit/github-6sydzv?file=integrations/preact.mjsI couldn't find any documentation or tests for the old renderer, so I'm not totally clear how it was used. I imagine you always need
@astrojs/preact
as well as compat, so thought this API would be nice:If that could work, I think it would be the simplest Preact compat support around?
Testing
Nate added an E2E test for the compat option
Docs
Chris updated the integration README to follow our new template and add details about the new
compat
config option.