-
Notifications
You must be signed in to change notification settings - Fork 802
feat(render): in-house formatter #2361
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
d6b8cc1
to
4c4a460
Compare
Just to comment here kudos for jumping into the problem with a comprehensive solution! |
24cd9c7
to
42d4c5b
Compare
615e633
to
8b201de
Compare
even though it has node in the name it actually runs fine in the browser
}); | ||
return printChildrenOf(root, { | ||
preserveLinebreaks: false, | ||
maxLineLength: 80, |
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.
I think this is an unexpecting behavior for #1847
Although prettier use whitespace sensitive formating https://prettier.io/blog/2018/11/07/1.15.0.html#whitespace-sensitive-formatting
When we are building email template, the max line is restrictly related to language/fontsize/layout and CSS attributes. I think enforcing an 80 max is a very opinionated decision - the in house formater also carry the same logic that wouldn't fix the issue.
Or any way we can have an option to disable auto line-break for the max length check?
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.
I'm not sure I 100% understand what you're asking, but the preserveLineBreak
being set to true should not wrap text at all. Maybe the option name is a bit confusing.
Also, the max line length is also configurable - 80 is just the default value.
For instance,
An email has multiple block/section in UI, we might not want a global max
length as the length could be different based on font size, UI layout or
others.
When we build email, we are assuming the content itself (text node) won’t
be modified that we can control over layout/css.
The pretty function itself is configurable but not directly exposed in the
preview server (or to the render function).
IMO prettify the html attribute make sense but not the content itself as
those line breaks does affect the rendering layout.
…On Fri, Jul 25, 2025 at 5:07 AM Gabriel Miranda ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/render/src/shared/utils/pretty.ts
<#2361 (comment)>:
>
- return doc;
- });
+ return printChildrenOf(root, {
+ preserveLinebreaks: false,
+ maxLineLength: 80,
I'm not sure I 100% understand what you're asking, but the
preserveLineBreak being set to true should not wrap text at all. Maybe
the option name is a bit confusing.
Also, the max line length is also configurable - 80 is just the default
value.
—
Reply to this email directly, view it on GitHub
<#2361 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5ZUUX6ZY7MXETSUFBGSN33KIMWZAVCNFSM6AAAAACBP3CFQGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTANJVGE2TIOJSGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
The current formatter (prettier) allows for us to format the resulting email templates in a way that feels familiar to users, but the downsides are:
So, this pull request shows off at least a good POC for a starting point for us to think about perhaps moving into our own in-house formatter and parser. We still want to parse the HTML's end result (in another pull request, perhaps) to avoid users sending invalid HTML, but this only includes a very lenient parser meant for roughly understanding the AST and printing it afterwards.
Having an in-house formatter also allows us to bend the end results to avoid email client quirks that wouldn't be really present in usual HTML.
(By the way, the majority of the additions in this pull request are the testing assets)
The other ecosystem options:
The original pull request in which we added prettier was #1777