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

feat(render,tailwind): render JSX to strings without relying on React DOM #36

Merged
merged 15 commits into from Oct 12, 2023

Conversation

phpnode
Copy link
Contributor

@phpnode phpnode commented Oct 9, 2023

Component / Package Name: jsx-to-string

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no
  • maybe, because this renderer doesn't support hooks or class components.

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.

@phpnode phpnode changed the title Add jsx-to-string package to render JSX to strings without relying on React DOM feat(jsx-to-string) render JSX to strings without relying on React DOM Oct 9, 2023
@phpnode phpnode changed the title feat(jsx-to-string) render JSX to strings without relying on React DOM feat(jsx-to-string): render JSX to strings without relying on React DOM Oct 9, 2023
@shellscape
Copy link
Owner

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:

  1. 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.

  2. 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.

  3. 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.

  4. 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).

  5. 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

  6. 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.

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.

@phpnode
Copy link
Contributor Author

phpnode commented Oct 9, 2023

  1. 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.

  1. 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.

  1. 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.

👍

  1. 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.

  1. 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

👍

  1. 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.

@phpnode
Copy link
Contributor Author

phpnode commented Oct 9, 2023

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.

@shellscape
Copy link
Owner

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.

@lordelogos
Copy link
Collaborator

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.

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!

@shellscape
Copy link
Owner

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.

@shellscape
Copy link
Owner

@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.

@shellscape shellscape changed the title feat(jsx-to-string): render JSX to strings without relying on React DOM feat(render,tailwind): render JSX to strings without relying on React DOM Oct 12, 2023
Copy link
Owner

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

This looks like it's good to go to me. Awesome stuff. @lordelogos please clone the fork and verify on your end. We have a few new tests against the demo templates in this PR now, which were created off of main so we can be somewhat confident that the old renderer output matches the new output.

Copy link
Collaborator

@lordelogos lordelogos left a comment

Choose a reason for hiding this comment

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

@phpnode This is awesome

@shellscape shellscape merged commit fd7651f into shellscape:main Oct 12, 2023
4 checks passed
@shellscape
Copy link
Owner

big win for the project thanks again

thanks!

@phpnode phpnode deleted the feat/jsx-to-string branch October 12, 2023 19:15
@shellscape
Copy link
Owner

@phpnode something I was thinking about today - do you know off hand if these changes support async components?

@phpnode
Copy link
Contributor Author

phpnode commented Nov 5, 2023

@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?

@shellscape
Copy link
Owner

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 honos stringifier and see how it aligns with the results of the prior work you did.

@shellscape
Copy link
Owner

@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<JSX.Element>?

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants