Prettier-ignore comments in `.astro` #26

drwpow · 2021-10-06T18:24:00Z

Changes

Gets  comments working in .astro.

There are probably more changes here than expected. Basically, the comment handling set off a few changes:

Handle comments in Prettier
Because comments were handled, Prettier can now manage whitespace around them (and can move them when needed).
Because Prettier now manages whitespace around comments differently, some of the hardlines we were inserting between documents had to be adjusted, and removed
Because I fiddled with the hardlines between documents, I ended up with extra whitespace within markdown codeblocks which count as separate “documents” (such as no double-line break between the end of --- and the first line of HTML).

I also made a few other whitespace changes that I can’t explain but they pass tests 🤷🏻

Testing

Tests should pass!

Docs

drwpow · 2021-10-06T18:24:29Z

src/printer.js

-  getText,
-  isObjEmpty,
+  trimTextNodeLeft,
+  trimTextNodeRight,


Sorry for noise; I just alphabetized these (and in a couple other places) to see what’s being imported/exported better.

drwpow · 2021-10-06T18:24:51Z

src/printer.js

@@ -44,7 +47,7 @@ const {
 * @param {import('prettier').Parseropts<any>} opts
 * @param {(path: import('prettier').AstPath<any>) => import('prettier').Doc} print
 */
-const printTopLevelParts = (node, path, opts, print) => {
+function printTopLevelParts(node, path, opts, print) {


Minor cleanup: trying to make everything consistent.

drwpow · 2021-10-06T18:25:30Z

src/printer.js

@@ -138,40 +152,43 @@ const print = (path, opts, print) => {
        return group(path.map(print, 'children'));
      }
    }
-    case 'Text':
-      if (!isPreTagContent(path)) {


This logic isn’t changed at all; I just flattened the if statements to be a little more readable as I was debugging this behavior.

drwpow · 2021-10-06T18:26:40Z

src/utils.js

@@ -528,37 +527,67 @@ const isObjEmpty = (obj) => {
  return true;
 };

+/** Recursively attach comments to nodes */
+function attachCommentsHTML(node) {


The core of this PR

drwpow · 2021-10-06T18:28:19Z

src/utils.js

+    }
+  }
+
+  // remove arbitrary whitespace nodes


I found this behavior necessary because Prettier kept trying to preserve the whitespace between the actual Prettier comment and the node it was attached to. But despite moving whitespace around, Prettier seemed to rearrange everything correctly in the following pass when it processes the comment

drwpow · 2021-10-06T18:29:23Z

test/fixtures/in/prettier-ignore-html.astro

@@ -5,14 +5,7 @@
 <!-- prettier-ignore -->
 <div        class="x">Two</div>

-<!-- prettier-ignore-attribute -->


These weren’t even working in the Playground! ☹️ So removed.

drwpow · 2021-10-06T18:31:22Z

test/fixtures/out/converts-to-shorthand.astro

@@ -2,5 +2,4 @@
 const style = "color:red;";
 let id = "#aloha";
 ---
-


These extra linebreaks were removed from our printer adding linebreaks between “documents” (with the code fence & HTML being treated as 2 documents).

If we want a line break back here, I think we should add it as the first child of the HTML document only in that instance; otherwise unconditionally adding linebreaks between documents sometimes results in too much whitespace when the full document is reassembled.

These newlines were intentional!

I think softline should be the solution to our problem, since it does as you suggest, collapsing to zero whitespace when unneeded.

I can give softline a try

drwpow · 2021-10-06T18:32:07Z

src/printer.js

@@ -64,18 +67,18 @@ const printTopLevelParts = (node, path, opts, print) => {
  }

  const docs = flatten([parts.frontmatter, ...parseSortOrder(opts.astroSortOrder).map((p) => parts[p])]).filter((doc) => '' !== doc);
-  return group([join(hardline, docs)]);


Removing the hardline cut down on too much whitespace in areas (see prev. comment).

Could this be changed to a softline rather than removing it entirely? The intention is to have a single newline of space between top-level parts of an Astro file

I found when prettier reassembles the docs, it automatically adds lines on its own. When I add hardline or softline here it just results in a double-space between --- and the first line of HTML.

If that’s the style I can change it back! It just didn’t seem like that‘s what we’re going for from the existing output tests.

But I might be missing something, or there might be a better way to do it. TBH I’m confused on when Prettier does inject a new line at the end of a document; I just know I had to use stripTrailingHardline() in places.

jasikpark · 2021-10-06T19:57:11Z

hopefully adding support for embedded formatting to codespans will fix that so that you don't exactly need that context?

drwpow · 2021-10-06T23:31:24Z

hopefully adding support for embedded formatting to codespans will fix that so that you don't exactly need that context?

Yeah I think so! I was able to hack it with opts.parentParser === 'markdown' but other than that, I can’t tell what’s actually a top-level Astro file (comments should be respected) vs a nested code block within markdown (comments should be ignored).

jasikpark

Sorry for the late, post-merge review, but I have a few comments. I didn't realize how many changes were occurring in this one PR!

jasikpark · 2021-10-08T16:49:07Z

.eslintrc

@@ -1,5 +1,5 @@
 {
-  "ignorePatterns": ["test/fixures/**"],


I had this setup to intentionally be linting our test files as well, this change prevents plugin:ava/reccomended from being useful. Could we revert this change?

I can revert that. I would just have to add an .eslintrc file to set a few rules differently for tests (for example, the current ESLint doesn’t like when you import devDependencies).

oh, is it complaining about importing ava? maybe we can turn that rule off for the specific places we import from a devdependency?

jasikpark · 2021-10-08T16:52:15Z

src/printer.js

@@ -64,18 +67,18 @@ const printTopLevelParts = (node, path, opts, print) => {
  }

  const docs = flatten([parts.frontmatter, ...parseSortOrder(opts.astroSortOrder).map((p) => parts[p])]).filter((doc) => '' !== doc);
-  return group([join(hardline, docs)]);


Could this be changed to a softline rather than removing it entirely? The intention is to have a single newline of space between top-level parts of an Astro file

jasikpark · 2021-10-08T16:54:17Z

src/printer.js

@@ -451,20 +473,25 @@ const embed = (path, print, textToDoc, opts) => {
    return textToDoc(node.content, { ...opts, parser: 'html' });
  }

-  return null;


Is there a reason to choose between returning null and "" here? I think there's one or two other places to change it if this improves things.

TBH I was a bit confused on this. I found that returning null in certain places broke the printer, so I just assumed that there was a problem with it. But it may just be a problem with our own printer! I can revert this.

jasikpark · 2021-10-08T16:55:27Z

src/utils.js

@@ -171,7 +170,7 @@ function printRaw(node, originalText, stripLeadingAndTrailingNewline = false) {
 }

 function isNodeWithChildren(node) {
-  return !!node.children;
+  return node && Array.isArray(node.children);


Good change! Will children always be an array?

It should be. This is more just protection because anything can maniplate the ast.

jasikpark · 2021-10-08T16:56:54Z

src/utils.js

+
+  const nodesToRemove = [];
+
+  // note: the .length - 1 is because we don’t need to read the last node


what does this mean? Isn't length - 1 normal syntax to read the entirety of a list? Wouldn't you need length - 2 to ignore the last node?

Ah yeah that’s confusing. < node.children.length reaches the end so < node.children.length - 1 stops 1 short. Can change to make this simpler.

ope lol i missed that bit..

jasikpark · 2021-10-08T16:58:52Z

test/fixtures/out/converts-to-shorthand.astro

@@ -2,5 +2,4 @@
 const style = "color:red;";
 let id = "#aloha";
 ---
-


These newlines were intentional!

I think softline should be the solution to our problem, since it does as you suggest, collapsing to zero whitespace when unneeded.

drwpow · 2021-10-08T17:21:21Z

Sorry for the late, post-merge review, but I have a few comments. I didn't realize how many changes were occurring in this one PR!

Please! Comments welcome. Will address all these in a follow-up, and happy to address anything else you find. I just didn’t want to put a full review on you. Happy to change anything here.

drwpow commented Oct 6, 2021

View reviewed changes

Attach HTML comments to AST

81d86a3

drwpow force-pushed the prettier-html-comment branch from 58587fd to 81d86a3 Compare October 6, 2021 23:26

drwpow marked this pull request as ready for review October 6, 2021 23:26

Separate test & lint jobs

17aa0ab

Fix lint errors

474ac12

drwpow changed the title ~~[wip] Prettier-ignore comments in .astro~~ Prettier-ignore comments in .astro Oct 6, 2021

drwpow requested a review from jasikpark October 6, 2021 23:38

drwpow merged commit 8b86f7d into main Oct 7, 2021

drwpow deleted the prettier-html-comment branch October 7, 2021 16:28

antonyfaris mentioned this pull request Oct 8, 2021

Fix test macro 'PrettierMarkdown' #30

Merged

jasikpark reviewed Oct 8, 2021

View reviewed changes

drwpow mentioned this pull request Oct 8, 2021

PR feedback #31

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prettier-ignore comments in `.astro` #26

Prettier-ignore comments in `.astro` #26

drwpow commented Oct 6, 2021 •

edited

Loading

drwpow Oct 6, 2021

drwpow Oct 6, 2021

drwpow Oct 6, 2021

drwpow Oct 6, 2021

drwpow Oct 6, 2021

drwpow Oct 6, 2021

drwpow Oct 6, 2021 •

edited

Loading

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

drwpow Oct 6, 2021

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

drwpow Oct 8, 2021

jasikpark commented Oct 6, 2021

drwpow commented Oct 6, 2021

jasikpark left a comment

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

jasikpark Oct 8, 2021

jasikpark Oct 8, 2021

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

jasikpark Oct 8, 2021

drwpow Oct 8, 2021

jasikpark Oct 8, 2021

jasikpark Oct 8, 2021

drwpow commented Oct 8, 2021


		const nodesToRemove = [];

		// note: the .length - 1 is because we don’t need to read the last node

Prettier-ignore comments in .astro #26

Prettier-ignore comments in .astro #26

Conversation

drwpow commented Oct 6, 2021 • edited Loading

Changes

Testing

Docs

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

drwpow Oct 6, 2021 • edited Loading

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

Choose a reason for hiding this comment

jasikpark commented Oct 6, 2021

drwpow commented Oct 6, 2021

jasikpark left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Oct 8, 2021

Prettier-ignore comments in `.astro` #26

Prettier-ignore comments in `.astro` #26

drwpow commented Oct 6, 2021 •

edited

Loading

drwpow Oct 6, 2021 •

edited

Loading