[lexical-markdown] Bug fix: skip format of the empty string on export #7979
Conversation
…en the string has leading and trailing whitespaces (facebook#7400)" This reverts commit 4d459e3. # Conflicts: # packages/lexical-markdown/src/__tests__/unit/LexicalMarkdown.test.ts # packages/lexical-markdown/src/importTextTransformers.ts
Leading and trailing whitespaces inside formatted text are already moved outside the formatted region to ensure correct markdown. But if the formatted text only contain whitespace, then we end up with formatting for the empty string, e.g. `**** ` for a bold whitespace. So we need to skip the formatting in these cases.
reinseth
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon,
takuyakanbr and
zurfyx
as code owners
|
Hi @reinseth! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
I think this may need a more targeted approach than a revert, the escaped entities do work in many cases, for example, here are the removed tests and how github renders them (all as expected) Hello
Hello world! Text boldstart text boldend text Text boldstart It It
|
|
That's unrelated to the original bug report in #6526 (Styling should be removed from the whitespace). You should consider doing that in a separate change to prevent mixing concerns. |
|
As a sidenote: As you can see, the last one breaks. Escaping leading/trailing/single whitespace/newline as an html entity is a hack that only partially work. |
|
I didn’t create the examples, they are from the test suite that’s being reverted in this PR. That’s why I’m suggesting that this should be a more targeted fix to specifically test and address the scenarios that don’t work. |
|
I don't follow you. #7400 only adds a single test case (the newline case) and then augments other cases with verification that whitespaces are escaped on export. And we don't want the whitespaces escaped. |
|
@etrepum: checking in. How would you like to proceed with this? I didn't quite follow what changes from the previous PR that you would like to retain. Maybe those aspects are properties of the markdown plugin that should be expressed in a different PR / feature? (considering that the original bug was that the markdown export produced |
|
As a Lexical user/consumer on a large-scale platform, I wanted to share a production perspective. Ever since the change in #7400, we had to revert to v0.28.0 of Lexical and are seriously considering patching the HTML-entity changes back to their original state if we upgrade again. This was due to restricted portability and rendering issues in Markdown workflows, which affected most of our Markdown to HTML parsers in our platform. This PR restores the expected behaviour, aligns with CommonMark, and feels like a much safer and more robust solution in the long run. Even with the reverted changes, we currently have to regex out |
|
The blocker here is that because it's changing tests that aren't problematic (listed above in comments) it needs a more thorough review that I simply have not had time for, and won't have time to do for at least another week. If it was a targeted fix that only affected serialization for incorrect markdown then it would be a quicker review. If another maintainer is interested they are welcome to participate here. |
|
Ok, good to know. You could review commit by commit, where the first commit is the revert, and the second commit is the actual change, which is a small and targeted change. |
|
It doesn’t really work like that, since the commit you’re reverting is released all of the changes have to be considered |
|
Hi, I'd like to see if this PR is going anywhere or it's in some sort of a "limbo" state? |
3 participants
Leading and trailing whitespaces inside formatted text are already moved outside the formatted region to ensure correct markdown. But if the formatted text only contain whitespace, then we end up with formatting for the empty string, e.g.
****for a bold whitespace. So we need to skip the formatting in these cases.The previous attempt at fixing this in #7400 used html entities to escape leading/trailing whitespaces inside formatted texts, but this breaks commonmark.
Example that breaks when using html entities:
https://spec.commonmark.org/dingus/?text=bold%20space%20with%20adjacent%20text%3A%20**%26%2332%3B**adjacent%0A%0A
Reverts #7400
Closes #7575
Test plan
Automated test in
LexicalMarkdown.test.tsBefore
After