Change custom request types to LSP commands #473
Conversation
LSP commands are the intended way to implement custom behaviour.
remcohaszing
added
🦋 type/enhancement
🦋 Changeset detectedLatest commit: 08da27f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
|
|
||
| export const implementations = { | ||
| 'mdx.toggleDelete': createSyntaxToggle('delete', '~'), | ||
| 'mdx.toggleEmphasis': createSyntaxToggle('emphasis', '_'), |
There was a problem hiding this comment.
It doesn’t really matter. I know remark stringifies uses * in its output by default. I personally feel * is a bit overloaded. Emphasis and strong text share the same characters, but * is also used for list items. Also Prettier uses _, which I think added to making that character option more popular.
TL;DR I prefer _, but we can change it to * if you feel very strongly about it. This should be done in a follow-up PR though, as it’s an unrelated change.
There was a problem hiding this comment.
Why does it not matter? Yes, it matters. _ and * get parsed differently. _ occurs more often in natural language.
There was a problem hiding this comment.
Both exist, but have relatively rare use cases in natural language. It’s really just a preference, and it’s ok your preference differs from mine.
I will change it in a follow-up PR anyway, as I believe the strongest argument is that it’s good to match the remark-stringify default. If we enable configurable formatting for #456, I think we should respect the same configuration optionn for syntax toggles.
| @@ -146,3 +139,12 @@ export function createSyntaxToggle(context, type, separator) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| export const implementations = { | |||
| 'mdx.toggleDelete': createSyntaxToggle('delete', '~'), | |||
There was a problem hiding this comment.
where’s this word from, delete? Perhaps strikethrough is better. Also, it’s a GFM-only feature. And, 2 tildes work there too. One tilde was forbidden by the GFM spec for a while, until recently, even though it worked.
There was a problem hiding this comment.
delete is the mdast node type. Based on your comment, I agree inserting 2 tildes is better. This changed be done in a follow-up PR though, as it’s an unrelated change.
There was a problem hiding this comment.
This doesn’t particularly have to match the node type, as it’s injecting into a document.
There was a problem hiding this comment.
It’s a toggle. It either injects or removes, depending on whether the cursor position matches a node of this type.
Yes, because of a timing issue. I think we were just lucky the tests weren’t failing before. I’ll look into it. |
|
The failing test is due to the fact that the language feature was never triggered on the test file and therefore the language service instance was not established. The issue has been fixed by volarjs/volar.js@e53022a. |
Commands now use a switch to differentiate between commands. Existing logic was too clever. The currently existing commands are similar, but future commands might not be. The new pattern is more flexible.
This means clients need to implement less custom logic.
It’s no longer used in production code.
|
I’ll merge this as-is and process the comments in follow-up PRs. |
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
👍 phase/yes
Initial checklist
Description of changes
LSP commands are the intended way to implement custom behaviour.