Closed phpnode closed 10 months ago
Thanks a lot for opening the PR. We've been looking at alternatives for a while now. A few thoughts that are up for discussion:
First thing is that we need to have a clear picture of what is copied source from hyperons, react, etc, and what is original code by you. If there's copied code and it's not an original work, we need to include (and ship) the appropriate licenses and credit the original authors. It's super important that we continue to be good stewards of open source in this regard.
I'd also like to have a clear picture of what is snipped from React's parser, particularly "React's string escaping." Please add some code comments at the top of the files that contain that, and please link back to the original source in the comment.
I'm not sure that this needs to be a separate package. This fits nicely into the render
package, since it's the same concern.
For the source that's coming from hyperons, is there a means to use their source from the package instead of copying? Legitimate question because I'm as yet unfamiliar with their source. If that answer is no, I'd like to approach them about opening up their API a bit to allow for the customizations we need, after this PR goes through (we can do that).
On renderAsync
; it looks like this is now performing the same operations as render
. We can safely remove render-async.ts
and add an additional renderAsync
export on render.ts
which wraps the call to render()
in a Promise
Test snapshots; what I'd like to do is implement the change for #3 and integrate into the render
package, and then run the tests as-is to see if there's any change in the HTML and Plain output. If there isn't then we're cooking with gas.
Lot's of comments above, but this is a big change and one we want to get right. Thanks again for taking the lead on this. If you'd like me to jump in and make the changes talked about above, please let me know. I'm happy to help push this forward.
- First thing is that we need to have a clear picture of what is copied source from hyperons, react, etc, and what is original code by you. If there's copied code and it's not an original work, we need to include (and ship) the appropriate licenses and credit the original authors. It's super important that we continue to be good stewards of open source in this regard.
Ok, from Hyperons I pinched renderToString stringifyStyles and the tests. I copied escapeHtml from React DOM as React escapes some things differently.
- I'd also like to have a clear picture of what is snipped from React's parser, particularly "React's string escaping." Please add some code comments at the top of the files that contain that, and please link back to the original source in the comment.
👍 will add a comment.
- I'm not sure that this needs to be a separate package. This fits nicely into the
render
package, since it's the same concern.
👍
- For the source that's coming from hyperons, is there a means to use their source from the package instead of copying? Legitimate question because I'm as yet unfamiliar with their source. If that answer is no, I'd like to approach them about opening up their API a bit to allow for the customizations we need, after this PR goes through (we can do that).
Not at the moment, they have a different representation for JSX elements. I assume the intent of this project at the moment is to continue to support a subset of React, rather than using a completely different JSX runtime? If my assumption is wrong then that would open up a lot of different possibilities, at the expense of breaking backwards compatibility.
- On
renderAsync
; it looks like this is now performing the same operations asrender
. We can safely removerender-async.ts
and add an additionalrenderAsync
export onrender.ts
which wraps the call torender()
in aPromise
👍
- Test snapshots; what I'd like to do is implement the change for chore(demo): setup documentation #3 and integrate into the
render
package, and then run the tests as-is to see if there's any change in the HTML and Plain output. If there isn't then we're cooking with gas.
I'm not sure what to do on this one?
Lot's of comments above, but this is a big change and one we want to get right. Thanks again for taking the lead on this. If you'd like me to jump in and make the changes talked about above, please let me know. I'm happy to help push this forward.
ah, I remembered why I made this a separate package - @jsx-email/tailwind
also uses React's renderToStaticMarkup
. We can make that dependent on the render
package instead, but we'll also have to export this jsxToString()
function and make that part of the render
package's API.
That extra export sounds good to me. No worries on the tests/snapshots. Let me know when I can step in and I'll handle that. Don't worry about failing CI in the mean time.
ah, I remembered why I made this a separate package -
@jsx-email/tailwind
also uses React'srenderToStaticMarkup
. We can make that dependent on therender
package instead, but we'll also have to export thisjsxToString()
function and make that part of therender
package's API.
Firstly, This is really amazing work. Thank you @phpnode
Secondly, I totally agree that this should be a part of render
and not a standalone package. I will look into the use of renderToStaticMarkup
in @jsx-email/tailwind
, There should be a better way.
Can't wait for this to ship!
This looks phenomenal. I'm going to pull it down and run through everything locally, but this looks ready to go. Will let you know if anything comes up locally. Really big step for the project, thanks a lot.
@phpnode I added some tests for the renderer upstream, and updated your branch to run the jsxToString on the same tests, and we've got some failures with moon render:test
where the tests previously passed using react. The new tests use the same templates that are in apps/demo
. Please have a look.
big win for the project thanks again
@phpnode something I was thinking about today - do you know off hand if these changes support async components?
@shellscape they don't support async at the moment. It's doable, but we'll need to decide what should happen if the caller uses render
(a synchronous API) when their tree contains an async component. Do we just throw? Do we return JSX.Element | Promise<JSX.Element>
? Do we deprecate it and make all renders async?
All good questions. My gut says that the option throwing an error for async render
when an async component is used is probably the right move, so we don't have to deprecate anything, and can maintain the API as it is.
For reference, hono
has an open PR that will apparently be merged soon to support this https://github.com/honojs/hono/pull/1630/files. It looks like they're taking the approach of returning a string or a promise. The addition of <Suspense/>
support has me thinking that it might be good to demo using hono
s stringifier and see how it aligns with the results of the prior work you did.
@phpnode I pushed a major version for cli
and render
that unifies the render()
method and makes it async.
Do we return JSX.Element | Promise
?
We can now :)
I tried plugging in hono
after that PR was merged but there are too many differences that aren't compatible with React to make it useful. I'd still like things to render in React if people want (even though I think it's silly at this point).
Seeing about what it would take to update jsxToString to support Suspense
and async components. If you've got time to poke at it too, that would be cool. That's gong to allow us to use unocss
for the Tailwind component (a huge deal) and also code highlighters like shikiji
which would also be awesome for a formatted Code component (which we got a request for via Discord the other day)
Component / Package Name: jsx-to-string
This PR contains:
Are tests included?
Breaking Changes?
If yes, please include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
This PR adds a separate package for rendering JSX to strings. It's based on https://github.com/i-like-robots/hyperons but uses React's string escaping and doesn't support some features such as context. This change is necessary because using React's
renderToStaticMarkup
is not permitted within React server components.