Pretty print wrapper.html() output

[tbuteler]

Mimics v1 wrapper.html() by using "pretty" on HTML output before returning it.

Pretty-printing of HTML was introduced in 1.0.0-beta.30, and this proposed implementation is based on what was previously there (introduced in this PR and commit).

I can imagine this is a somewhat controversial subject, so I wouldn't be opposed to doing away with pretty-printing altogether, as long as it's a documented breaking change with the upgrade to v2.

Fixes #498.

[cexbrayat]

Thanks for the PR.

Before giving it a proper review, I'm wondering if that should not be an option? wrapper.html({ pretty: true}) for example.
Or the other way around if the prettified version is preferred by default.

What do you think ?

[tbuteler]

I think it would be a good improvement from v1 if it was finally an option, yes. So if that's what we want I'm happy to give it another pass when I have the time.

[afontcu]

Hi! This looks like a great addition, even though it is a breaking change. Now, we're in Release Candidate, so we're still "on time" if we want to introduce a breaking change. To avoid it, we could add it as an opt-in feature, but then we'd need to document the difference between VTU and VTU-next.

Thus, I'm in favor of introducing this breaking change here 👍

[cexbrayat]

I would be in favor of introducing it, but as an option. As you can see, it does change a lot of our tests to add meaningless spaces and line returns. And if we introduce it by default, it is a breaking change for everybody using vtu-next, whereas as an opt-in feature, it allows to upgrade from but v1 to v2 easily. But I understand the concern about having to document this option.

What does the rest of the team think?

[lmiller1990]

I think we should match v1 as closely as possible. That behavior is what most people will be expecting. @cexbrayat points out this is a breaking change for next users... but how many people are really asserting against a specifically formatted HTML string? I guess the main breaking change is for snapshots. Likely we should build a snapshot serializer to handle snapshot specific things (which wouldn't care if the input HTML is pretty or not).

I am sure this has been discussed many times, like here and here, including { pretty: true }. Although always good to revisit and scrutinize past decisions.

Pretty HTML is easier to read, generally, but awkward to assert against (eg, the changes to the specs in this PR). I wonder if we should just inline snapshots instead for those tests? Seems like our tests are a bit bad - relying on a specifically formatted HTML string, when whitespace has no meaning in HTML, seems like a code smell.

Anyway as for the PR, if this gives us closer parity with v1, we should definitely do it. A breaking change in RC is not ideal, but a v1 -> v2 breaking change is much more significant, imo. What do you guys think? I consider v1 to be the reference implementation (including all of the weird, undocumented behavior people have come to rely on) so we should default to that unless there is a very compelling reason to break things.

TL;DR: looks good to me, pretty by default makes sense, if someone has a really good reason to want unpretty html we can consider it, but I can't think of one.

[lmiller1990]

Happy with this one for now, will merge once the conflict is fixed! Thanks!

Happy to explore { pretty: true } options later - either way, the behavior won't change, we will default to pretty html since this is the behavior in v1.

[tbuteler]

@lmiller1990 sounds good to me. That should be the last conflict sorted.

[cexbrayat]

@tbuteler 👍 Would you mind rebasing your PR on master and squashing all commits together please? This guarantees a nice Git history. (I can do it if you aren't familiar with this)

[tbuteler]

@cexbrayat wouldn't it suffice to do a squash merge?

[cexbrayat]

The Github UI was complaining about conflicts, but it's all good now: squashed and merged 👍
Thanks for your contribution.