Resolve rule of fill violation in JSX and add assertion. #16700

seiyab · 2024-09-28T02:00:40Z

Description

Checklist

I’ve added tests to confirm my change works.
- Should we have test for assertDocFill()? -> done
~~(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).~~
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

seiyab · 2024-09-30T05:23:24Z

src/language-js/print/jsx.js

  const rawJsxWhitespace = options.singleQuote ? "{' '}" : '{" "}';
  const jsxWhitespace = isMdxBlock
-    ? " "
+    ? line


I'm worrying that it might not be just a careless mistake. I'm not familiar with MDX at all...

fisker · 2024-09-30T07:52:13Z

src/document/utils/assert-doc.js

      };

-export { assertDoc, assertDocArray };
+const assertDocFill =


assertDocFillParts, since it only accept parts not fill

fisker · 2024-09-30T07:57:42Z

src/document/utils/assert-doc.js

+        hasUnexpectedString = true;
+        return;
+      case DOC_TYPE_IF_BREAK:
+        traverseDoc(doc.breakContents, rec);


Is this still needed if we use the last argument in outer traverse ?

prettier/src/document/utils/traverse-doc.js

Line 24 in 655eb7b

function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) {

Maybe yes. shouldTraverseConditoinalGroups doesn't affect ifBreak(), where we want to modify the behavior.

prettier/src/document/builders.js

Lines 120 to 132 in 94fa86e

function ifBreak(breakContents, flatContents = "", opts = {}) {

assertDoc(breakContents);

if (flatContents !== "") {

assertDoc(flatContents);

}

return {

type: DOC_TYPE_IF_BREAK,

breakContents,

flatContents,

groupId: opts.groupId,

};

}

prettier/src/document/utils/traverse-doc.js

Lines 68 to 76 in 1ea2fd0

case DOC_TYPE_GROUP:

if (shouldTraverseConditionalGroups && doc.expandedStates) {

for (let ic = doc.expandedStates.length, i = ic - 1; i >= 0; --i) {

docsStack.push(doc.expandedStates[i]);

}

} else {

docsStack.push(doc.contents);

}

break;

prettier/src/document/utils/traverse-doc.js

Lines 64 to 66 in 1ea2fd0

case DOC_TYPE_IF_BREAK:

docsStack.push(doc.flatContents, doc.breakContents);

break;

Or do I misunderstand?

…rule-of-fill

seiyab · 2024-09-30T22:23:31Z

src/document/utils/assert-doc.js

+            throw new Error(
+              `Unexpected non-line-break doc at ${i}. Doc type is ${type}.`,
+            );


Using InvalidDocError is not straightforward because InvalidDocError considers such documents valid. Error message gets weird and very hard to understand (message: "doc is valid").
I use just Error here.

seiyab · 2024-11-04T13:20:15Z

@fisker ping (no hurry)

fisker · 2024-11-24T04:41:12Z

I'm really sorry for the long delay, I was super busy recently.

Change looks good, the only reason I didn't approve is that I'm not sure about using cleanDoc, generally it should be avoid but I'm not sure if it's really necessary.

fisker · 2024-11-25T13:50:00Z

src/document/utils.js

 }

+/**
+ * returns true iff cleanDoc(doc) === ""


Suggested change

* returns true iff cleanDoc(doc) === ""

* returns true if cleanDoc(doc) === ""

It's intentional. I mean "if and only if"
https://en.wikipedia.org/wiki/If_and_only_if

"if and only if" (often shortened as "iff")

src/document/utils/assert-doc.js

Co-authored-by: fisker Cheung <[email protected]>

pkg-pr-new · 2024-12-21T09:05:56Z

Open in Stackblitz

npm i https://pkg.pr.new/prettier@16700

commit: ae937dc

seiyab added 9 commits September 28, 2024 10:07

add stricter assertion into fill()

027d021

resolve rule of fill issue. some comments and naming can be problematic.

72d899f

cleanup

6a4d9fe

update comments

4dd4081

rename printWhitespace -> printStringOrLine

0f59092

refine isValidSeparator() a bit

16390b1

remove FIXME comment for fixed issue

d6e3be5

fix idempotency problem in MDX

5f311ac

add changelog

655eb7b

seiyab commented Sep 30, 2024

View reviewed changes

seiyab marked this pull request as ready for review September 30, 2024 05:47

fisker reviewed Sep 30, 2024

View reviewed changes

seiyab added 3 commits October 1, 2024 07:01

Merge remote-tracking branch 'origin/main' into jsx-children-satisfy-…

04c4350

…rule-of-fill

rename assertDocFill() -> assertDocFillParts()

2d748aa

add test for assertDocFillParts()

2fb5c16

seiyab commented Sep 30, 2024

View reviewed changes

seiyab requested a review from fisker October 1, 2024 02:51

seiyab added 3 commits October 13, 2024 19:17

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

ea8da29

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

52e94c7

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

78dda9e

seiyab added 2 commits November 14, 2024 07:45

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

1f7a779

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

a4ee6aa

replace cleanDoc(doc) === "" by isEmptyDoc(doc)

41176e5

fisker reviewed Nov 25, 2024

View reviewed changes

fisker approved these changes Nov 25, 2024

View reviewed changes

fisker reviewed Nov 25, 2024

View reviewed changes

src/document/utils/assert-doc.js Outdated Show resolved Hide resolved

Update src/document/utils/assert-doc.js

07df60e

Co-authored-by: fisker Cheung <[email protected]>

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

72a734c

Merge branch 'main' into jsx-children-satisfy-rule-of-fill

ae937dc

fisker requested a review from sosukesuzuki January 20, 2025 06:28

fisker added this to the 3.5 milestone Jan 20, 2025

sosukesuzuki approved these changes Jan 21, 2025

View reviewed changes

sosukesuzuki merged commit 97d8115 into prettier:main Jan 21, 2025
31 checks passed

seiyab deleted the jsx-children-satisfy-rule-of-fill branch January 21, 2025 04:22

	function ifBreak(breakContents, flatContents = "", opts = {}) {
	assertDoc(breakContents);
	if (flatContents !== "") {
	assertDoc(flatContents);
	}

	return {
	type: DOC_TYPE_IF_BREAK,
	breakContents,
	flatContents,
	groupId: opts.groupId,
	};
	}

	case DOC_TYPE_GROUP:
	if (shouldTraverseConditionalGroups && doc.expandedStates) {
	for (let ic = doc.expandedStates.length, i = ic - 1; i >= 0; --i) {
	docsStack.push(doc.expandedStates[i]);
	}
	} else {
	docsStack.push(doc.contents);
	}
	break;

	case DOC_TYPE_IF_BREAK:
	docsStack.push(doc.flatContents, doc.breakContents);
	break;

	* returns true iff cleanDoc(doc) === ""
	* returns true if cleanDoc(doc) === ""

Uh oh!

Resolve rule of fill violation in JSX and add assertion. #16700

Resolve rule of fill violation in JSX and add assertion. #16700

Uh oh!

Conversation

seiyab commented Sep 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

seiyab Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

fisker Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

fisker Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

seiyab Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seiyab Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

seiyab commented Nov 4, 2024

Uh oh!

fisker commented Nov 24, 2024

Uh oh!

fisker Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

seiyab Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

fisker Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pkg-pr-new bot commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seiyab commented Sep 28, 2024 •

edited

Loading

seiyab Sep 30, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 21, 2024 •

edited

Loading