Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape roundtrip #366

Merged
merged 13 commits into from
Feb 4, 2025
Merged

Conversation

threefjefff
Copy link
Contributor

@threefjefff threefjefff commented Jan 16, 2025

Closes [#234]

Treat the start of inlining a parent block the same way as the begining of a line (to catch sequences where, for example, we've fought with the editor in rich text mode to allow us to insert a # without it turning into a header, thereby serialising to - \#)

PR Checklist

  • All new/changed functionality includes unit and (optionally) e2e tests as appropriate
  • All new/changed functions have /** ... */ docs
  • I've added the bug/enhancement and other labels as appropriate

Environment(s) tested

  • Device: descktop
  • OS: Windows
  • Browser: Chrome
  • Version 132.0.6834.160 (Official Build) (64-bit) (cohort: Stable)

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
axelboc Axel Bocciarelli
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Thanks @threefjefff for the PR.
Overall I think the PR is going in the right direction. 🎉

You should add a unit test for the preserve-escape rule and see if it is worth adding a regression test for the issue Dion reported. Perhaps in the markdown-serializer unit test file.

Great work so far! You can see how much code you ended up writing to fix a relatively simple bug - this is why we should be intentional going forward about what is worth fixing and what is not. More code = more maintenance, more cognitive overhead, more bugs potential, etc...

For this specific bug I think we can go ahead and merge once the PR is in a good state with its relevant tests. Thanks for now. 🙂

Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

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

Thanks for the bug fix @threefjefff. I ran npm run format to resolve some of the formatting inconsistencies and get the lint job passing (8c7a3e0). Other than that, the only things I think necessary to get this merged is what Giamir mentioned.

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Thanks @threefjefff for adding the tests.

As I was reviewing the tests I realized that original issue reported seems to be fixed exclusively by the state.atBlockStart expression in the markdown serializer you have added:

const startOfLine: boolean = state.atBlank() || state.atBlockStart || state.closed;

If that's the case we could reduce significantly the amount of code we will need to merge in. We could just add that expression and keep the roundtrip.e2e.test you created and call it done.

I don't know if I am missing some edge cases; as far as I can tell the tests seem to confirm my theory of the bug being fixed by simply adding state.atBlockStart. 🤷

Also sorry for realizing this only now.

[String.raw`\# not a header`],
[String.raw`- \# list item (not header)`],
] as const) {
test(`should make markdown -> richtext -> markdown round trip '${markdown}'`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test title is making the test output less readable because it honours the \n special char. A way to solve that is to do something like this:

`should make markdown -> richtext -> markdown round trip ${JSON.stringify(markdown)}`

Screenshot 2025-02-04 at 10 15 34

["lol\n\n***\n\nlmao"],
["`code`"],
//TODO: Codeblock does weird things roundtripping: Adds an extra space
//['```javascript\ncodeblock\n```' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Good knowledge for a potential follow up ticket (in case it is worth fixing of course).

@threefjefff
Copy link
Contributor Author

threefjefff commented Feb 4, 2025

As I was reviewing the tests I realized that original issue reported seems to be fixed exclusively by the state.atBlockStart expression in the markdown serializer you have added:

Great catch! I think this is one of those things where I started fixing things and then was blinded by the implementation. Such is the benefit of a second pair of eyes 🤦‍♂️

I don't know if I am missing some edge cases; as far as I can tell the tests seem to confirm my theory of the bug being fixed by simply adding state.atBlockStart. 🤷

Certainly not for this case. We were discussing these "feel bad" roundtrip errors the other day, and it might be that what's here is a precursor for some of those, but if so I can always refer back to earlier states of this PR.
Much further down the line, we might want to also styalize and add hover behaviour for escape chars, but that's very much not a priority at the moment.

I'll get to stripping this back to the essentials and get back to you.

@threefjefff threefjefff requested a review from giamir February 4, 2025 10:16
@threefjefff threefjefff marked this pull request as ready for review February 4, 2025 10:16
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Great stuff @threefjefff.
I am happy we could sort this issue with less than a line of code in the end. 🎉

I am merging this in.

@giamir giamir merged commit 8de12d2 into StackExchange:main Feb 4, 2025
4 checks passed
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.

None yet

3 participants