-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Change custom request types to LSP commands #473
Changes from 2 commits
535fa9c
7e982e6
07cd18f
d1e4e81
cf1da61
08da27f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@mdx-js/language-service': minor | ||
'@mdx-js/language-server': minor | ||
--- | ||
|
||
Convert the custom MDX syntax toggle request types into LSP commands. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,4 @@ | ||
/** | ||
* @typedef {import('./lib/commands.js').SyntaxToggleParams} SyntaxToggleParams | ||
* @typedef {import('./lib/service-plugin.js').Commands} Commands | ||
*/ | ||
|
||
export {commands} from './lib/commands.js' | ||
export {createMdxLanguagePlugin} from './lib/language-plugin.js' | ||
export {createMdxServicePlugin} from './lib/service-plugin.js' | ||
export {resolveRemarkPlugins} from './lib/tsconfig.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,20 +3,15 @@ | |
* @import {Nodes} from 'mdast' | ||
*/ | ||
|
||
/** | ||
* @typedef SyntaxToggleParams | ||
* The request parameters for LSP toggle requests. | ||
* @property {string} uri | ||
* The URI of the document the request is for. | ||
* @property {Range} range | ||
* The range that is selected by the user. | ||
*/ | ||
|
||
/** | ||
* @callback SyntaxToggle | ||
* A function to toggle prose markdown syntax based on the AST. | ||
* @param {SyntaxToggleParams} params | ||
* The input parameters from the LSP request. | ||
* @param {LanguageServiceContext} context | ||
* The Volar service context. | ||
* @param {string} uri | ||
* The URI of the document the request is for. | ||
* @param {Range} range | ||
* The range that is selected by the user. | ||
* @returns {TextEdit[] | undefined} | ||
* LSP text edits that should be made. | ||
*/ | ||
|
@@ -29,17 +24,15 @@ import {VirtualMdxCode} from './virtual-code.js' | |
/** | ||
* Create a function to toggle prose syntax based on the AST. | ||
* | ||
* @param {LanguageServiceContext} context | ||
* The Volar service context. | ||
* @param {Nodes['type']} type | ||
* The type of the mdast node to toggle. | ||
* @param {string} separator | ||
* The mdast node separator to insert. | ||
* @returns {SyntaxToggle} | ||
* An LSP based syntax toggle function. | ||
*/ | ||
export function createSyntaxToggle(context, type, separator) { | ||
return ({range, uri}) => { | ||
function createSyntaxToggle(type, separator) { | ||
return (context, uri, range) => { | ||
const parsedUri = URI.parse(uri) | ||
const sourceScript = context.language.scripts.get(parsedUri) | ||
const root = sourceScript?.generated?.root | ||
|
@@ -146,3 +139,12 @@ export function createSyntaxToggle(context, type, separator) { | |
} | ||
} | ||
} | ||
|
||
export const implementations = { | ||
'mdx.toggleDelete': createSyntaxToggle('delete', '~'), | ||
'mdx.toggleEmphasis': createSyntaxToggle('emphasis', '_'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn’t really matter. I know remark stringifies uses TL;DR I prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does it not matter? Yes, it matters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
'mdx.toggleInlineCode': createSyntaxToggle('inlineCode', '`'), | ||
'mdx.toggleStrong': createSyntaxToggle('strong', '**') | ||
} | ||
|
||
export const commands = Object.keys(implementations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where’s this word from,
delete
? Perhapsstrikethrough
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn’t particularly have to match the node type, as it’s injecting into a document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a toggle. It either injects or removes, depending on whether the cursor position matches a node of this type.