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

perf: minify runtime comments, remove unnecessary attr quotes #1557

Merged
merged 2 commits into from
Apr 21, 2020

Conversation

DylanPiercey
Copy link
Contributor

Description

Working toward: #1432

Checklist:

This PR removes unnecessary characters from the html output where possible:

  1. Removes -- from comments since all tested browsers do not require them, for example <!M> is a treated valid comment.
  2. Removes unnecessary quotes around attributes where possible. Any attribute that doesn't contain '"> or any space characters can have the quotes removed. Technically the spec says that =, < and backticks also need to be escaped, however browsers appear happy to ignore that. The implementation could do better by ensuring the minimum number of escapes by switching between single or double quotes, but for performance reasons instead it will immediately use single quotes when the first double quote is found which works well for most cases.
  • I have read the CONTRIBUTING document and have signed (or will sign) the CLA.
  • I have updated/added documentation affected by my changes.
  • I have added tests to cover my changes.

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #1557 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1557   +/-   ##
=======================================
  Coverage   90.54%   90.54%           
=======================================
  Files         355      355           
  Lines       12759    12759           
=======================================
  Hits        11552    11552           
  Misses       1207     1207           
Impacted Files Coverage Δ
...ges/marko/src/runtime/components/beginComponent.js 100.00% <100.00%> (ø)
...kages/marko/src/runtime/components/endComponent.js 100.00% <100.00%> (ø)
packages/marko/src/runtime/html/AsyncStream.js 89.41% <100.00%> (ø)
packages/marko/src/runtime/html/helpers/attr.js 100.00% <100.00%> (ø)
...es/marko/src/runtime/html/helpers/escape-quotes.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2002ff6...759f7d5. Read the comment docs.

@DylanPiercey DylanPiercey merged commit 7a6f802 into master Apr 21, 2020
@DylanPiercey DylanPiercey deleted the minify-runtime-comments-and-attr-quotes branch April 21, 2020 20:29
DylanPiercey added a commit that referenced this pull request Apr 23, 2020
@redonkulus
Copy link

@DylanPiercey FYI, this ended up breaking marko snapshots locally since all the attrs were removed. I thought it was a bug in my code somewhere. It probably didn't warrant a major version bump, but can definitely cause builds to fail until updated.

@DylanPiercey
Copy link
Contributor Author

@redonkulus at the end of the day a snapshot test is going to capture many implementation details of Marko that we don't consider a "public api". We'd expect snapshots to need updating across many releases in Marko 4. The point of snapshot tests though are not to assert that something is working, but to let you know that something has changed. I think developers having to update snapshots during changes like this is acceptable.

@tigt
Copy link
Contributor

tigt commented Apr 24, 2020

Removing the -- delimiters from comments produces a lot of noisy errors/warnings from tools like the HTML validator for our automated & manual checks. Not sure how much that matters to you.

@mlrawlings
Copy link
Member

mlrawlings commented Apr 24, 2020

@tigt We may need to revisit that...

It technically non-conforming, but the spec does define that it should be parsed as a comment:
https://html.spec.whatwg.org/multipage/parsing.html#markup-declaration-open-state
https://html.spec.whatwg.org/multipage/parsing.html#bogus-comment-state

And we tested it in Safari, FF, Chrome, Edge, and IE: it parses as expected

@tigt
Copy link
Contributor

tigt commented Apr 24, 2020

Yeah, I’m not aware of any HTML parsers that would refuse to work with it.

@DylanPiercey
Copy link
Contributor Author

It's tricky. Basically the shorter comments are objectively better since it's just less rendered and shipped code, but tools are going to complain.

I think Marko needs to take a stance on if it is more valuable to have output which linters like, or that is more performant / smaller.

@mlrawlings
Copy link
Member

We're reverting the comment change for now. We'll revisit this in the future.

@alexnaidovich
Copy link

alexnaidovich commented May 13, 2021

Is there a way to bring the quotes for attributes' values (that were considered there to be unnecessary) back to the output? There are real cases when quotes are necessary. Maybe, are there some configurable options for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants