Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

expose _RenderingContext.emptyContext #45

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

alephao
Copy link
Contributor

@alephao alephao commented Sep 25, 2024

This change makes the static var emptyContext public, so I can use it in my custom renderer.

That's necessary because when creating a custom renderer, It's impossible to run HTML._render since one of its arguments is a _RenderingContext and it doesn't expose initializers.

@sliemeobn
Copy link
Owner

I mean, I guess there is no harm in this, but for more fully supporting "custom rendering" outside this package we'd need to think strategy.

also, be aware that underscored stuff might be broken (as it is by convention not part of the API contract)

can I ask what you are implementing? is it something that should go in the package?

@alephao
Copy link
Contributor Author

alephao commented Sep 25, 2024

can I ask what you are implementing?

Sure, I'm implementing a renderer that outputs a more snapshot-friendly html, like this.

I use snapshot testing for 100% of my HTML tests and also HTTP tests that outputs HTML, and the default pretty format isn't very snapshot friendly compared to the example above. For instance, if I have an <input> with a bunch of attributes, and later decide to change only one of the attributes, my snapshot tests would show that only the placeholder line changed.

Example diffs:

With the default pretty print

- <input a="b" b="c" c="l" d="e">
+ <input a="b" b="c" c="I" d="e">

With diff-friendly

<input a="b"
       b="c"
-      c="l"
+      c="I"
       d="e">

is it something that should go in the package?

IDK if it fits the core lib, my use-case is to use this specifically with the swift-snapshot-testing library.

@sliemeobn
Copy link
Owner

I see, I can tell you that writing a correct and efficient html-formatter is quite tough ; )
I essentially gave up, because the current pretty-printer has bugs and I thought about removing it.

however, I think your use-case is probably the best reason to have something like pretty-print at all, so I would not mind if we add a formatting option to the renderFormatted() that fits your needs. I could see this being useful for many people.

if you are up for it that is, I have no issue with this one change here.

@sliemeobn
Copy link
Owner

sliemeobn commented Sep 25, 2024

to clarify, the tricky bit is that adding line breaks within "inline" elements (say between <span> or <i>) would produce a different output, because the browser interprets a line break as whitespace and adds a blank basically...

@alephao
Copy link
Contributor Author

alephao commented Sep 25, 2024

to clarify, the tricky bit is that adding line breaks within "inline" elements (say between or ) would produce a different output, because the browser interprets a line break as whitespace and adds a blank basically...

That's one thing I don't really care for my use-case, which makes the implementation much easier. I don't want the renderer to try doing it in an optimal way that cares about browsers quirks. I'm only using it for snapshot tests, not for rendering on the browser. In fact, I'll never send that HTML to the browser, not sure why anyone would.

What I need is to keep the rendered html as close as possible to what is actually sent, but adding a bunch of predictable break-lines to help with diff reading. If I remove/add spaces to my html to fix some weird rendering, the snapshot will have that extra/removed space in the diff as well, that's the behavior I want.

What do you think? If you agree with that approach, then I already have this like 80% implemented in my fork, I can send another PR soon

@sliemeobn
Copy link
Owner

I'll merge your change, we can always figure out how to deal with formatting in the core package later.

@sliemeobn sliemeobn changed the title feat: expose _RenderingContext.emptyContext expose _RenderingContext.emptyContext Sep 26, 2024
@sliemeobn sliemeobn merged commit 5994b0c into sliemeobn:main Sep 26, 2024
5 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants