Skip to content

Conversation

@fisker
Copy link
Member

@fisker fisker commented Dec 8, 2020

Description

Fix #9503

Checklist

  • I’ve added tests to confirm my change works.
  • (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.

Try the playground for this PR

@fisker fisker mentioned this pull request Dec 8, 2020
4 tasks
test <World /> test
</Hello>123
</Hello>
123
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change may related to #6961

@fisker fisker changed the title [WIP] Refactor markdown paragraph print Refactor markdown paragraph print Dec 17, 2020
@fisker fisker force-pushed the markdown-paragraph branch from fb4e2c4 to 232bef1 Compare December 17, 2020 02:52
@fisker fisker marked this pull request as ready for review December 17, 2020 03:24
fisker added 2 commits January 4, 2021 10:16
# Conflicts:
#	src/language-markdown/printer-markdown.js
# Conflicts:
#	src/language-markdown/printer-markdown.js
Base automatically changed from master to main January 23, 2021 17:13
@fisker
Copy link
Member Author

fisker commented Jan 29, 2021

@thorn0 Can you review this, when you got time? This is actually quite big change, because it called unsafe normalizeDoc on "Root", and I removed it. But maybe normalizeDoc was safe before we extract to doc-utils.js

@thorn0
Copy link
Member

thorn0 commented Jan 29, 2021

Ooops, I keep forgetting about this one...

@thorn0
Copy link
Member

thorn0 commented Jan 31, 2021

The doc for #9503 still doesn't look right:

[
  fill([
    conditionalGroup([
      group([/*...*/]),
      group([/*...*/]),
    ]),
    " ", // <--- should be `line`
    "is",
    line,
// ...

if (typeof printed === "string") {
return printed;
}
return fill(getDocParts(printed));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the result of cleanDoc to fill is wrong. cleanDoc doesn't guarantee that the array has the expected structure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, and it seems I've done something wrong in other places, I'll check.

@thorn0
Copy link
Member

thorn0 commented Jan 31, 2021

Yes, this is a big one. My research plan so far:

MDX: line break inserted between elements and text in JSX blocks

<asdf/>zxcv

  • Prettier 2.2.1 parses it all as JSX (not Markdown) and prints line breaks the way it normally does for JSX.
  • That's probably not the expected output. That's why MDX: line break inserted between elements and text in JSX blocks #6961 was opened.
  • Also there is a fill bug that causes v2.2.1 to print <Hello></Hello><Hello></Hello>123 without that break.
    [
      fill([
        group(["<Hello", indent([]), ">"]),
        "</Hello>", // <--- Problem! Odd-indexed elements must be line breaks.
        hardlineWithoutBreakParent,
        breakParent,
        group(["<Hello", indent([]), ">"]),
        "</Hello>",
        softline,
        "123"
      ]),
      hardline,
    ]
  • Things to look into:
    1. The fill bug
      1. Is the bug in Inline React components in MDX cause line lengths to exceed print-width #9503 same or different?
      2. How did it work in older versions?
      3. Was normalizeDoc safe before it got extracted to doc-utils?
      4. Manipulations with normalizeDoc and cleanDoc (in particular, PR Refactor markdown paragraph print #9847) look like they might affect more things in Markdown than just MDX. What else can be affected?
    2. What actually is the expected output?
      1. Is the whitespace between the tag and the text significant?
      2. Should the text be parsed as JSX or as Markdown?

@fisker fisker mentioned this pull request Feb 3, 2021
4 tasks
# Conflicts:
#	src/language-markdown/printer-markdown.js
…markdown-paragraph

# Conflicts:
#	src/language-markdown/printer-markdown.js
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.

Inline React components in MDX cause line lengths to exceed print-width

2 participants