-
-
Notifications
You must be signed in to change notification settings - Fork 615
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(jsx/server): introduce jsx/dom/server
module for compatibility with react-dom/server
#2930
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2930 +/- ##
==========================================
+ Coverage 94.42% 94.45% +0.03%
==========================================
Files 136 137 +1
Lines 13343 13424 +81
Branches 2219 2236 +17
==========================================
+ Hits 12599 12680 +81
Misses 744 744 ☔ View full report in Codecov by Sentry. |
4e76943
to
7d551fc
Compare
jsx/server
module for compatibility with react-dom/server
jsx/server
module for compatibility with react-dom/server
… with `react-dom/server`
…eam for compatibility
7d551fc
to
e750cff
Compare
jsx/server
module for compatibility with react-dom/server
jsx/dom/server
module for compatibility with react-dom/server
@yusukebe Would you please review? |
Hi @usualoma ! I don't know if it will be the best way but have you ever thought about implementing |
@yusukebe Thanks for your comment! In my opinion, the best way to do this is to "not include any implementation in @hono/react-compat". The only code included is re-export in “@hono/react-compat". export * from 'hono/jsx'. But I also see your point. You mean that this PR code is "just a wrapper for React compatibility, it doesn't add any new value", so does it really need to be added to "hono"? It could be used in "vite.config.ts" or "honox", but, well, I don't have any concrete ideas at the moment. I know there are different opinions, but I would prefer this code to be in "hono" instead of "@hono/react-compat". |
Oh, just to say, "@hono/react-compat" is currently aliased to "hono/jsx", but if it is aliased to "hono/jsx/dom" and aimed at something "client-side only, but very very small", it is possible that this pull request's implementation is not needed in the first place. |
Hi @usualoma
Yeah, definitely. But it's okay to merge this PR! It was not a big problem for me. With this PR, the code in |
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.
Can you update jsr.json
too?
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!
@usualoma Thanks! I'll leave this not merging because this is a |
Implement React compatible
renderToString
andrenderToReadableStream
.This component is used to create an alias in #2925
Is the name
jsx/dom/server
appropriate?As it is a component that provides stringify function, I don't think it is related to “DOM", but I chose this name because it is easier to understand if it corresponds to "react-dom/server".
The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code