[WIP] feat: html support #4753

alexander-akait · 2018-06-26T13:11:28Z

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:

Indent head and body tags inside html or not
Indent inside script and style tags or not
Insert newline after script and style tags or not

j-f1 · 2018-06-26T13:14:47Z

tests/html_basics/__snapshots__/jsfmt.spec.js.snap

+      placeholder="Enter email">
+
+    <small id="emailHelp"
+      class="form-text text-muted">We'll never share your email with anyone else.


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

@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 · 2018-06-26T15:43:30Z

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.

alexander-akait · 2018-06-26T16:03:42Z

@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 · 2018-06-26T19:14:01Z

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

j-f1 · 2018-06-26T19:20:17Z

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

gabrielmaldi · 2018-06-26T21:46:17Z

@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 · 2018-06-26T21:54:02Z

@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.

alexander-akait · 2018-06-26T22:15:13Z

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

alexander-akait · 2018-06-26T22:31:51Z

@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 · 2018-06-26T22:40:49Z

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

vjeux · 2018-06-27T01:19:13Z

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

vjeux · 2018-06-27T01:36:03Z

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 · 2018-06-27T01:37:30Z

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.

YarnSphere · 2018-06-27T08:05:11Z

@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.

alexander-akait · 2018-06-27T08:35:30Z

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

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

Any objections?

alexander-akait · 2018-06-27T11:06:36Z

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

alexander-akait · 2018-06-27T11:55:18Z

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>

vjeux · 2018-06-27T16:19:20Z

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.

alexander-akait · 2018-06-27T16:20:16Z

@vjeux ok

vjeux · 2018-06-27T20:15:55Z

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

alexander-akait · 2018-09-12T09:26:48Z

@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.

ikatyang · 2018-09-12T11:20:58Z

src/language-html/printer-htmlparser2.js

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


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.

ikatyang · 2018-09-12T11:23:26Z

src/language-html/parser-parse5.js

@@ -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 can be handled in other PR.)

ikatyang · 2018-09-12T11:26:36Z

src/language-html/printer-htmlparser2.js

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


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

ikatyang · 2018-09-12T11:28:51Z

src/language-html/printer-htmlparser2.js

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


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

ikatyang · 2018-09-12T11:37:15Z

src/language-html/printer-htmlparser2.js

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


Is there a reference for these attributes?

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

Thanks! Added in a5cecbd.

alexander-akait · 2018-09-12T14:16:02Z

@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 · 2018-09-12T15:19:43Z

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

j-f1 · 2018-09-12T15:33:56Z

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

ikatyang · 2018-09-14T02:29:44Z

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

danburzo · 2018-09-14T15:15:18Z

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.

alexander-akait · 2018-09-14T15:26:02Z

@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> Can't detect original text inikulin/parse5#261
more tests
maybe switch to fork of parse5 to support *case tags (<MyTag>) (link on fork somewhere here)

andrewvmail · 2018-09-15T01:20:41Z

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 · 2018-09-15T15:06:17Z

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

Context: #4753 (comment)

alexander-akait added the status:wip Pull requests that are a work in progress and should not be merged label Jun 26, 2018

alexander-akait mentioned this pull request Jun 26, 2018

Supporting HTML #1882

Closed

j-f1 previously requested changes Jun 26, 2018

View reviewed changes

alexander-akait force-pushed the feat-html-support branch from 0de972e to ef5e75c Compare June 26, 2018 15:04

alexander-akait force-pushed the feat-html-support branch from ef5e75c to cce54f6 Compare June 26, 2018 18:31

alexander-akait force-pushed the feat-html-support branch 2 times, most recently from 00e5ab4 to 00426cd Compare June 26, 2018 21:20

alexander-akait force-pushed the feat-html-support branch 2 times, most recently from fa6a696 to 21480de Compare June 26, 2018 22:12

j-f1 mentioned this pull request Jun 27, 2018

1.13.x breaks eslint-plugin-html/eslint-plugin-prettier #4759

Closed

lipis mentioned this pull request Jun 27, 2018

Prettier 2.0 (old) #3503

Closed

alexander-akait added 8 commits September 11, 2018 18:41

fix: print pre and textarea in right way

c73d8c6

tests: xhtml doctype

4821849

fix: use > instead />

2efc698

tests: case

303e081

refactor: remove toLowerCase()

363fc90

feat: support prettier-ignore

810a0bd

tests: empty script and style tags

6fafe5a

fix: clean

e2e0239

alexander-akait force-pushed the feat-html-support branch from 42cddd7 to b2c368a Compare September 11, 2018 15:44

tests: refactor and more

61f81a1

alexander-akait force-pushed the feat-html-support branch from b2c368a to 61f81a1 Compare September 11, 2018 18:18

ikatyang reviewed Sep 12, 2018

View reviewed changes

suchipi merged commit 8b0bdf5 into master Sep 13, 2018

alexander-akait deleted the feat-html-support branch September 13, 2018 13:11

StarpTech mentioned this pull request Sep 15, 2018

HTML TODOs #5098

Closed

26 tasks

ikatyang mentioned this pull request Sep 16, 2018

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

Merged

2 tasks

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

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

79d510c

Context: #4753 (comment)

ikatyang mentioned this pull request Sep 23, 2018

feat(html): tweak printer #5134

Closed

4 tasks

ikatyang added this to the 1.15 milestone Oct 25, 2018

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 23, 2019

lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019

[WIP] feat: html support #4753

[WIP] feat: html support #4753

Conversation

alexander-akait commented Jun 26, 2018 • edited by ikatyang

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lydell commented Jun 26, 2018

alexander-akait commented Jun 26, 2018

gabrielmaldi commented Jun 26, 2018

j-f1 commented Jun 26, 2018

gabrielmaldi commented Jun 26, 2018

j-f1 commented Jun 26, 2018

alexander-akait commented Jun 26, 2018

alexander-akait commented Jun 26, 2018

lipis commented Jun 26, 2018 • edited

vjeux commented Jun 27, 2018

vjeux commented Jun 27, 2018 • edited by j-f1

vjeux commented Jun 27, 2018

YarnSphere commented Jun 27, 2018 • edited

alexander-akait commented Jun 27, 2018

alexander-akait commented Jun 27, 2018 • edited by j-f1

alexander-akait commented Jun 27, 2018 • edited by j-f1

vjeux commented Jun 27, 2018

alexander-akait commented Jun 27, 2018

vjeux commented Jun 27, 2018

alexander-akait commented Sep 12, 2018 • edited

Choose a reason for hiding this comment

ikatyang Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Sep 12, 2018

ikatyang commented Sep 12, 2018

j-f1 commented Sep 12, 2018

ikatyang commented Sep 14, 2018

danburzo commented Sep 14, 2018

alexander-akait commented Sep 14, 2018 • edited

andrewvmail commented Sep 15, 2018

ikatyang commented Sep 15, 2018

alexander-akait commented Jun 26, 2018 •

edited by ikatyang

lipis commented Jun 26, 2018 •

edited

vjeux commented Jun 27, 2018 •

edited by j-f1

YarnSphere commented Jun 27, 2018 •

edited

alexander-akait commented Jun 27, 2018 •

edited by j-f1

alexander-akait commented Jun 27, 2018 •

edited by j-f1

alexander-akait commented Sep 12, 2018 •

edited

ikatyang Sep 12, 2018 •

edited

alexander-akait commented Sep 14, 2018 •

edited