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.

Sign up for GitHub

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

Jump to bottom

refactor: render package #1750

Open
wants to merge 11 commits into
base: canary
Choose a base branch
from
Open

refactor: render package #1750

wants to merge 11 commits into from

Conversation

vcapretz
Copy link
Member

some refactor in the render package to make it more future proof, this also removes the previously deprecated renderAsync method, so the goal is that this ends up in a new major version

@vcapretz vcapretz self-assigned this Oct 30, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 1989630

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

@vercel Vercel
Copy link

vercel bot commented Oct 30, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-email ❌ Failed (Inspect) Nov 19, 2024 10:01pm
react-email-demo ❌ Failed (Inspect) Nov 19, 2024 10:01pm

gabrielmfern
gabrielmfern reviewed Oct 30, 2024
View reviewed changes
packages/render/src/browser/render.tsx
import { Suspense } from "react";
import { pretty } from "../shared/utils/pretty";
import { renderToReadableStream } from "react-dom/server";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Saw you removed the dynamic import back into this, are we sure that this isn't going to bring #871 back?

Copy link
Member Author

Choose a reason for hiding this comment

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

good call, I'm going to double check.
my concern with the dynamic import was that it was not reliably resolving the correct version of the react-dom/server package, doing it from the root seems to solve the issue, but I'll check if it's not introducing any regressions

@gabrielmfern gabrielmfern force-pushed the canary branch 2 times, most recently from 8e9f8a2 to e62ec22 Compare November 4, 2024 16:33
@gabrielmfern gabrielmfern force-pushed the canary branch 3 times, most recently from 66c5ab9 to 9f5b6d3 Compare November 8, 2024 16:46
@gabrielmfern gabrielmfern force-pushed the canary branch 2 times, most recently from 4ee4d1d to 0187b0a Compare December 23, 2024 13:28
@gabrielmfern gabrielmfern force-pushed the canary branch 2 times, most recently from bf3f899 to 1748335 Compare January 13, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielmfern gabrielmfern gabrielmfern left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@vcapretz vcapretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vcapretz @gabrielmfern