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.

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

feat: implement template compiler options 'trimTextWhitespace' and 'collapseTextWhitespace' (#3934) #7598

Conversation

cerlestes
Copy link

Implements #3934

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
The added options have to be made accessible to at least vue-loader in order to enable them for our builds. I'll see if I can implement that in another PR.

@cerlestes
Copy link
Author

cerlestes commented Feb 3, 2018

After turning on those options, my production build sizes have decreased by 4-10% without any negative side effects at all.

Enabling the trimTextWhitespace option will trim away whitespace characters around each text node:

// before
_c('p',[_vm._v("\r\n\t\tText     \t\n\t    Line 1\r\n\t\t"), ...])
_c('p',[_vm._v("\r\n\t\tText   Line 2\r\n\t\t"), ...])

// after
_c('p',[_vm._v("Text     \t\n\t    Line 1"), ...])
_c('p',[_vm._v("Text   Line 2"), ...])

Enabling the collapseTextWhitespace option will collapse sequences of multiple whitespace characters within the whole text node (also at the beginning and end), similiarly to what a browser does:

// before
_c('p',[_vm._v("\r\n\t\tText     \t\n\t    Line 1\r\n\t\t"), ...])
_c('p',[_vm._v("\r\n\t\tText   Line 2\r\n\t\t"), ...])

// after
_c('p',[_vm._v(" Text\nLine 1 "), ...])
_c('p',[_vm._v(" Text Line 2 "), ...])

Enabling both options will obviously result in the best outcome, with all whitespace reduced to a minimum:

// before
_c('p',[_vm._v("\r\n\t\tText     \t\n\t    Line 1\r\n\t\t"), ...])
_c('p',[_vm._v("\r\n\t\tText   Line 2\r\n\t\t"), ...])

// after
_c('p',[_vm._v("Text\nLine 1"), ...])
_c('p',[_vm._v("Text Line 2"), ...])

vue-loader#1151 has a PR for pulling in those options from the webpack configuration, and I have a second commit waiting, which would enable setting these two options and the already existing preserveWhitespace on a per-SFC-file basis:

<template whitespaces="not-preserve,trim,collapse">
	<div>
		Text Line 1
		<p>
			Text Line 2

		</p>
		<p>
			Text	Line 3
		</p>
		Text    Line 4
		Text Line 5
	</div>
</template>

Ideally these options should be set to true by default, but my PR has them set to false so that it's non-breaking, even non-changing at all.
Edit: As described in below comments, only the collapse option can be safely turned on; enabling the trim option might lead to unwanted behaviour with inline elements.

I'm looking forward to hearing what you guys think.

@kennardconsulting
Copy link

Excited to see this! Great work!

@econic
Copy link

econic commented Feb 23, 2018

Really looking forward to it!

Just one wish:
I think this should not remove whitespaces, but replace them with a single space.
Like this you don't have issues where the browser renders a whitespace.
E.g. text node 1 .. inline node .. text node 2 (all as siblings)
Here the browser will render text node 1's trailing whitespace and text node 2's leading whitespace.
If you trim each whitespace to a single space, the behaviour is not changed at all.

@cerlestes
Copy link
Author

cerlestes commented Mar 2, 2018

@econic Thanks for the input. You're totally right that trimming would be fatal in that case. If you only enable the collapse setting and not the trim one though, it should work out fine, as the whitespaces around the text nodes will be collapsed down to a single ASCII-space and not be completely removed.

If this PR is ever accepted, it'd probably be a safe bet to enable the collapse option by default, as this really shouldn't change behaviour, but reduce component size a lot. The trim option might break existing code and should stay off for now, and has a rather negligible effect any way; the major reduction occurs by collapsing "\t\t\t\t\t\t" down to just " " many times (depending on your project size).

@revolter
Copy link

revolter commented Nov 6, 2018

Any news on this one?

@Dewep
Copy link

Dewep commented Nov 27, 2018

Possible temporary solution: #3934 (comment)

@yyx990803
Copy link
Member

Thanks for the PR and sorry for letting it hang! See #9208 (comment) for something that achieves similar result with slight differences. We do want to keep the number of options to a minimum and avoid having too many possible different combinations of behavior. The new condense mode essentially combines collapse with a more conservative trim.

@yyx990803 yyx990803 closed this Dec 26, 2018
@afwn90cj93201nixr2e1re
Copy link

@yyx990803 its not same, coz it's doesnt remove tabulating, we dont need tabulating in compiled templates for example in frameworks.
изображение

@gold
Copy link

gold commented Feb 27, 2021

This overly complicated implementation seems to be based on a slight misunderstand of the default behavior of HTML rendering. Except for pre tag rendering, 2+ space characters are automatically collapsed to a single space in display. All developers know this behavior and thus format their HTML tags accordingly, e.g.;

<span>one</span>
<span>two</span>
<span>three</span>

knowning that it will render: one two three

One would not expect it to render: onetwothree

I wrote an HTML minifier in python many years ago; and it takes into account that some spacing between tags are significant and thus should never be completely removed. It's not perfect -- but it also does not cause rendering errors.

# release build strips out HTML comments and removes insignificant space
removeHtmlComments_rx = re.compile(r'<!--.+?-->', re.MULTILINE | re.DOTALL)
removeBlankLines_rx = re.compile(r'^\s*$\n', re.MULTILINE)
removeIndents_rx = re.compile(r'^\s+', re.MULTILINE)

The regexen are applied in these three sequential steps against HTML content. Done. Simplicity is a virtue.

Could this be improved? Yes, by replacing newlines with a single space in a fourth step perhaps.

(I recall something I recently read: "Perfect is sometimes the enemy of good.")

@cerlestes cerlestes deleted the feature/template-compiler-whitespace-options-3934 branch February 28, 2021 09:44
@cerlestes
Copy link
Author

@gold Your proposed implementation is exactly identical to the originally proposed "overly complicated" implementation. Even the regular expressions are basically identical.

@cerlestes cerlestes restored the feature/template-compiler-whitespace-options-3934 branch February 28, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants