Stop adding whitespace when parsing our html

[lmatiolis]

Type of PR (feature, enhancement, bug fix, etc.)

Bug fix.

Description

As shown by @shepmaster on #487, we were adding extra whitespace
to the DOM when parsing. That was causing morphdom to do several
replaces on the first page load since it was comparing a page without
the whitespace and the "new" page with all the whitespaces added.

The solution was just saying for Nokogiri to use the DEFAULT_HTML
but without doing the FORMAT part.

Fixes #487

Why should this be added

With the extra whitespace being added, we were re-rendering several parts of the DOM without it really being different. It was unnecessary re-renders and, for a more specific issue, it was causing the browser to close its autocomplete on a field cause it was replacing the field with a new one even though they were still the exact same DOM (but with extra whitespace).

Checklist

• My code follows the style guidelines of this project
• Checks (StandardRB & Prettier-Standard) are passing
• This is not a documentation update

[lmatiolis]

Please let me know if I should rebase the commits after a review. Thanks! o/

[leastbad]

Hey @lmatiolis I'm finally preparing to dive deep into this PR - thanks again and sorry for the slow reaction time.

Can you run standardrb and make sure the tests are passing, please?

[leastbad]

This could be a silly question/approach, but...

[1] pry(#<StimulusReflex::PageBroadcaster>)> Nokogiri::XML::Node::SaveOptions::DEFAULT_HTML
=> 71
[2] pry(#<StimulusReflex::PageBroadcaster>)> Nokogiri::XML::Node::SaveOptions::FORMAT
=> 1
[3] pry(#<StimulusReflex::PageBroadcaster>)> (Nokogiri::XML::Node::SaveOptions::DEFAULT_HTML & ~Nokogiri::XML::Node::SaveOptions::FORMAT)
=> 70
[4] pry(#<StimulusReflex::PageBroadcaster>)> (Nokogiri::XML::Node::SaveOptions::DEFAULT_HTML & ~Nokogiri::XML::Node::SaveOptions::FORMAT).class
=> Integer

Doesn't this mean that we could just call:

html = document.css(selector).inner_html(save_with: 70)

It just seems performative if we know that the return value of that method is always going to be 70... doesn't it?

[leastbad]

Hey, sorry for pushing ahead without waiting for reply. We're working really hard to get a release ready, and I wanted to make sure that this was in v3.5.

Could you give this a shot and verify that it solves your issue, please?

[lmatiolis]

Hey, sorry for pushing ahead without waiting for reply.

No worries at all! I'm glad we're moving this forward, no need to apologize o/

Doesn't this mean that we could just call:
html = document.css(selector).inner_html(save_with: 70)
It just seems performative if we know that the return value of that method is always going to be 70... doesn't it?

Yes, the end result will still be the same, but I think there's a risk involved that we're relying on a constant that's not us that are defining? Nokogiri/libxml2 could change that value (probably not likely, but it's a possibility) and everything would break for us. Using Nokogiri constant, we guarantee that if that's changed, they will update their constant to keep using the proper value.

I think that using just a "random" number like that would at least need a comment of some sort, cause we're losing readability on what that is and where it's coming from.

You mentioned performance, but I think that overhead is minimal on this case, no? I suppose we could instead do a constant ourselves instead of a function call too:

DEFAULT_HTML_WITHOUT_FORMAT = Nokogiri::XML::Node::SaveOptions::DEFAULT_HTML & ~Nokogiri::XML::Node::SaveOptions::FORMAT

Let me know your thoughts on this, I can test it once we decide the best approach.

Thanks!

[marcoroth]

I'm with @lmatiolis on this one. I think we should keep the DEFAULT_HTML and FORMAT constants Nokogiri provides.
With that we don't have a random number floating around which someone might ask: "why is it 70 and where is it coming from?". Plus it would still work if Nokogiri changes anything in the future.

[leastbad]

You two are both right. Sometimes I let myself forget that today, only me and god understand what 70 is for. In six months, it'll just be god.

@lmatiolis do you want to fix it up? I don't want to step on your toes any further 😀

[lmatiolis]

Thanks @marcoroth and @leastbad !

Yeah, no worries! I'll fix it up here.

[lmatiolis]

Hi @marcoroth and @leastbad , I've rebased on master and I've included one commit to do the test changes you've added, @leastbad . I also did the constant instead of the method that I was doing before cause I think that's better.

Let me know if everything is good! I did run bundle exec rake test_ruby and standardrb --fix, so hopefully it's everything as expected.

Thanks!

[leastbad]

Great detective work and congrats on your first PR to the project! I hope that it's the first of many.

[marcoroth]

Thank you so much for your contribution, Leandro! 🚀

[lmatiolis]

Thanks @leastbad and @marcoroth , we will keep working with stimulus reflex here and hope to contribute more in the future to this awesome project! o/

PS: I'm getting the praise here, but honestly, this was all @shepmaster 's work that I just drove to the finish line! <3

Thanks!