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

[WIP] feat: html support #4753

Merged
merged 26 commits into from Sep 13, 2018

Conversation

Projects
None yet
@evilebottnawi
Member

evilebottnawi commented Jun 26, 2018

Partial #1882

Feedback welcome.

How to use:

yarn add --dev prettier/prettier#feat-html-support
npm i -D prettier/prettier#feat-html-support

NOTE: you need to pass --parser parse5 to enable it.

Initial style preference questions:

  1. Indent head and body tags inside html or not
  2. Indent inside script and style tags or not
  3. Insert newline after script and style tags or not
placeholder="Enter email">
<small id="emailHelp"
class="form-text text-muted">We'll never share your email with anyone else.

This comment has been minimized.

@j-f1

j-f1 Jun 26, 2018

Member

This shouldn’t be broken onto multiple lines unless it goes over the print width.

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 26, 2018

Member

@j-f1 I know that there are a lot of problems, so I will assign a review from the main developers when I finish this, now it's just for feedback

@lydell

This comment has been minimized.

Collaborator

lydell commented Jun 26, 2018

What is the plan for handling whitespace? (I guess the code tells, but I'm too lazy.) In theory we can't change anything, because white-space: pre; and element.textContent. But that would be sad.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 26, 2018

@lydell looks like we should output this as is, it is impossible to solve, also i think we should release html asap, it is allow to get feedback from people and maybe we have new ideas how to solve this

@gabrielmaldi

This comment has been minimized.

gabrielmaldi commented Jun 26, 2018

Simple instructions on how to try this? I'd happily provide feedback after formatting my codebase.

@j-f1

This comment has been minimized.

Member

j-f1 commented Jun 26, 2018

@gabrielmaldi yarn add --dev prettier/prettier#feat-html-support or npm i -D prettier/prettier#feat-html-support.

@gabrielmaldi

This comment has been minimized.

gabrielmaldi commented Jun 26, 2018

@j-f1 thanks! I'm constantly getting [error] Invalid ``--write`` value. Expected a boolean, but received `[null,true]`. and I can't seem to get around it. This doesn't happen with prettier 1.13.6 (without this PR); could it be that something's wrong with --write or am I just running it wrong?

@j-f1

This comment has been minimized.

Member

j-f1 commented Jun 26, 2018

@evilebottnawi Would it be possible for you to avoid squashing your commits? It makes it harder to see what’s been changed since earlier and review those changes. Also, GitHub automatically squashed commits together when we merge the PR, so it doesn’t help keep the commit history any cleaner than it already is.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 26, 2018

@j-f1 👍, just squash PRs whats is look very bad (all first PRs look very bad 😄 ), no squash after last PR

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 26, 2018

@j-f1 Can we rename html_glimmer and html_handlebars to handlebars_glimmer and handlebars, because it is not html also for easy testing using node_modules/.bin/jest tests/html_*/ -u?

@lipis

This comment has been minimized.

Member

lipis commented Jun 26, 2018

Vote 👍 for indenting head and body tag inside the html and 👎 for not.

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

@j-f1 feel free to rename it to just glimmer, it’s no longer exactly compatible with handlebars

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

For whitespace, we should figure out a way to opt-out for specific places. The trick I used to do was:

<ul><!--
  --><li>First</li><!--
  --><li>Second</li><!--
--></ul>

It’s not pretty but it let me indent things without introducing whitespace when needed.

The second thing we need to have a documentation page about it and put a lot of warnings saying that unlike js, pretty printing a whole html folder is not safe because of this issue.

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

One tool that would be useful is to print all the places where prettier would add/remove whitespace in an easily digestible format so that you can manually inspect them when doing the big codemod.

@nunocastromartins

This comment has been minimized.

nunocastromartins commented Jun 27, 2018

@vjeux I don't know if it matters, but your trick introduces new comment nodes to the DOM tree, which may not be desired if you're performing DOM manipulations via JavaScript. An alternative, which I usually use, is:

<ul
  ><li>First</li
  ><li>Second</li
></ul>

Not the prettiest thing, but it shouldn't have any side-effect, afaik.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

@vjeux I agree about note in some case prettify is no safe, especially when you use white-space: pre. My solution:

  1. By default do not format children nodes inside pre tag.
  2. Add note about this.
  3. Use prettier-ignore as workaround when you want ignore formatting inside tag.

Any objections?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

Vote 👍 for indenting inside script and style tag and 👎 for not.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

Vote 👍 for insert newline after script and style tags and 👎 for not or ❤️ print as in source

<script>
alert("test");
</script>

vs

<script>alert("test");</script>

@lipis lipis referenced this pull request Jun 27, 2018

Open

Prettier 2.0 #3503

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

Let's avoid this voting game. Can you make decisions based on what you believe should be done and then we can revisit based on feedback we get and running in existing codebases.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

@vjeux ok

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

I forgot about prettier-ignore. This is indeed a good solution for whitespace in those cases.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

@vjeux should we respect xhtml?

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

What do you mean exactly by xhtml?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 27, 2018

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Jun 27, 2018

I mean, which part of xhtml do you have in mind?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 28, 2018

@vjeux if doctype is xhtml use <br />, if not use <br>

@lipis

This comment has been minimized.

Member

lipis commented Jun 28, 2018

To answer the first question: Twitter poll. Let's indent them all.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Sep 12, 2018

@ikatyang Can we merge this PR as is? I remove since for disable html by default. Don't have time on this right now, maybe other developers want to continue work.

(isLineNext(children[1]) || isEmpty(children[1]))
) {
children.shift();
children.shift();

This comment has been minimized.

@ikatyang

ikatyang Sep 12, 2018

Member

Could you add a test for this double shift thing? I'm not sure why we need it; removing [1] and the second shift does not affect any snapshot.

@@ -3,12 +3,15 @@
function parse(text /*, parsers, opts*/) {
// Inline the require to avoid loading all the JS if we don't use it
const parse5 = require("parse5");
const htmlparser2TreeAdapter = require("parse5-htmlparser2-tree-adapter");
try {
const isFragment = !/^\s*<(!doctype|html|head|body|!--)/i.test(text);

This comment has been minimized.

@ikatyang

ikatyang Sep 12, 2018

Member

⬆️ (This can be handled in other PR.)

: indent(children),
n.children.length ? concat([softline, "</", n.name, ">"]) : hardline
children.forEach(child => {
multilineChildren.push(child);

This comment has been minimized.

@ikatyang

ikatyang Sep 12, 2018

Member

multilineChildren seems unnecessary, it's just an alias for children.

if (!parentNode || !parentNode.sourceCodeLocation) {
return n.key;
}

This comment has been minimized.

@ikatyang

ikatyang Sep 12, 2018

Member

I'm not sure if it's possible to trigger, could you add a test for it?

return (
node.type === "attribute" &&
[
"allowfullscreen",

This comment has been minimized.

@ikatyang

ikatyang Sep 12, 2018

Member

Is there a reference for these attributes?

This comment has been minimized.

@thorn0

thorn0 Sep 14, 2018

Contributor

They're marked as "Boolean attribute" here:
https://html.spec.whatwg.org/multipage/indices.html#attributes-3

This comment has been minimized.

@ikatyang

ikatyang Sep 15, 2018

Member

Thanks! Added in a5cecbd.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Sep 12, 2018

@ikatyang i am glad to fix your comments, but i really don't have time 😞 I think other developers can solve this and add more tests

@ikatyang

This comment has been minimized.

Member

ikatyang commented Sep 12, 2018

I think it should be fine to merge, @j-f1 any thoughts?

@j-f1

This comment has been minimized.

Member

j-f1 commented Sep 12, 2018

If we’re not putting it in the production version, let’s merge!

@suchipi suchipi merged commit 8b0bdf5 into master Sep 13, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 96.59% of diff hit (target 80%)
Details
codecov/project 96.27% (+0.02%) compared to b020a56
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@evilebottnawi evilebottnawi deleted the feat-html-support branch Sep 13, 2018

@ikatyang

This comment has been minimized.

Member

ikatyang commented Sep 14, 2018

@evilebottnawi Is there a list for what haven't been done here? It'd be helpful for future work.

@danburzo

This comment has been minimized.

danburzo commented Sep 14, 2018

I've tried using this to prettify some Ractive templates, but it seems it converts all elements to lowercase? e.g. <ProfileCard> becomes <profilecard> which does not have the same meaning.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Sep 14, 2018

@ikatyang no, but known problems:

  • <template> tag (need fix output and more tests)
  • formatting css in <style></style> tags
  • problem with <p>I will display €</p> inikulin/parse5#261
  • more tests
  • maybe switch to fork of parse5 to support *case tags (<MyTag>) (link on fork somewhere here)
@andrewvmail

This comment has been minimized.

andrewvmail commented Sep 15, 2018

Not sure if this is even supposed to work for frameworks template yet but i tried with angular templates
<ion-segment [(ngModel)]="tab" color="secondary">
turned to
<ion-segment [(ngmodel)]="tab" color="secondary">

other than that i think its working!

@ikatyang

This comment has been minimized.

Member

ikatyang commented Sep 15, 2018

Opened #5098 to track HTML issues that should be done before the public release, please comment there if you find something unexpected.

@StarpTech StarpTech referenced this pull request Sep 15, 2018

Closed

HTML TODOs #5098

17 of 26 tasks complete

@ikatyang ikatyang referenced this pull request Sep 16, 2018

Merged

fix(html): exclude comments from fragment detection #5100

2 of 2 tasks complete

ikatyang added a commit that referenced this pull request Sep 17, 2018

@ikatyang ikatyang referenced this pull request Sep 23, 2018

Closed

feat(html): tweak printer #5134

4 of 4 tasks complete

@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment