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

Rewrite to use remark-language-server #65

Merged
merged 54 commits into from
Jul 13, 2022
Merged

Rewrite to use remark-language-server #65

merged 54 commits into from
Jul 13, 2022

Conversation

remcohaszing
Copy link
Member

@remcohaszing remcohaszing commented Dec 15, 2021

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This migrates the extension to use remark-language-server which is introduced
in remarkjs/remark#924.

This basically reimplements the extension from scratch andrepurposes it.

Old behavior:

  • Format code using a custom command.

New behavior:

  • Use the same configuration as remark-cli
  • Register an actual formatter
  • Support ESM configurations
  • Support diagnostics
  • Support quick fixes

Closes #17
Closes #41
Closes #58
Closes #61

@remcohaszing remcohaszing mentioned this pull request Dec 15, 2021
4 tasks
@ChristianMurphy ChristianMurphy requested review from asbjornu, mrmlnc and a team December 15, 2021 14:01
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @remcohaszing!
It looks like CI is getting stuck at

npm run clean && npm run lint && npm run compile

npm ERR! Missing script: "clean"

src/extension.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Dec 16, 2021

👍 after build succeeds!

@remcohaszing remcohaszing marked this pull request as draft December 24, 2021 13:18
@remcohaszing remcohaszing marked this pull request as ready for review December 27, 2021 17:47
This migrates the extension to use remark-language-server which is introduced
in remarkjs/remark#924.

This basically reimplements the extension from scratch andrepurposes it.

Old behavior:

- Format code using a custom command.

New behavior:

- Use the same configuration as remark-cli
- Register an actual formatter
- Support ESM configurations
- Support diagnostics
This uses esbuild to bundle and minify the extension.
It has also been renamed to lower case to be consistent with the rest of
the unified ecosystem.
'Marker style should be `*`',
'Marker style should be `*`',
'Marker style should be `*`'
])
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 is more about testing the language server can be loaded than about testing its functionality, as that’s tested upstream in unified-language-server.

package.json Outdated
"scripts": {
"clean": "rimraf out",
"build:language-server": "esbuild node_modules/remark-language-server/index.js --bundle --platform=node --minify --format=cjs --outfile=out/remark-language-server.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

ESBuild can’t process require statements which load NodeJS builtins, so the language server is built as a CJS module. It is able to use dynamic import() this way, so everything works fine this way (this is also tested).

/**
* @returns {Promise<void>}
*/
module.exports.run = () =>
Copy link
Member Author

Choose a reason for hiding this comment

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

The test file must be loadable by @vscode/test-electron, meaning it has to use CJS.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Only really prose nits!

.vscode/launch.json Show resolved Hide resolved
src/extension.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@wooorm wooorm changed the title Use remark-language-server Use remark-language-server Dec 27, 2021
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

wip review

package.json Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Co-authored-by: Titus <tituswormer@gmail.com>
This adds support for quick fixes.
@wooorm
Copy link
Member

wooorm commented Jan 13, 2022

@remcohaszing I see you sometimes pushing updates to dependencies. Are you treating this PR is ready to go? Because I’m waiting for the tests to turn green and I was under the impression that there are still some things being worked on?

@remcohaszing
Copy link
Member Author

I was hoping that might automagically fix it, but it didn’t, not that I really expected that.

Previously the Windows build failed, but now even the Linux build fails. It used to work on Linux.

Manual testing shows the following error.

Debugger listening on ws://127.0.0.1:6009/f22fc09c-2ff5-4353-ab6a-fab913fbf799
For help, see: https://nodejs.org/en/docs/inspector
[Error - 12:08:42 PM] Connection to server is erroring. Shutting down server.
/home/remco/Projects/vscode-remark/out/remark-language-server.js:25
${JSON.stringify(w.error,void 0,4)}`):i.error("Received response message without id. No further error information provided.");else{let E=String(w.id),h=x[E];if(Os(w,h),h){delete x[E];try{if(w.error){let b=w.error;h.reject(new z.ResponseError(b.code,b.message,b.data))}else if(w.result!==void 0)h.resolve(w.result);else throw new Error("Should never happen.")}catch(b){b.message?i.error(`Response handler '${h.method}' failed with message: ${b.message}`):i.error(`Response handler '${h.method}' failed unexpectedly.`)}}}}function ui(w){if(Qe())return;let E,h;if(w.method===co.type.method)h=b=>{let _=b.id,D=v[String(_)];D&&D.cancel()};else{let b=p[w.method];b&&(h=b.handler,E=b.type)}if(h||f)try{Ts(w),h?w.params===void 0?(E!==void 0&&E.numberOfParams!==0&&E.parameterStructures!==z.ParameterStructures.byName&&i.error(`Notification ${w.method} defines ${E.numberOfParams} params but recevied none.`),h()):Array.isArray(w.params)?(E!==void 0&&(E.parameterStructures===z.ParameterStructures.byName&&i.error(`Notification ${w.method} defines parameters by name but received parameters by position`),E.numberOfParams!==w.params.length&&i.error(`Notification ${w.method} defines ${E.numberOfParams} params but received ${w.params.length} argumennts`)),h(...w.params)):(E!==void 0&&E.parameterStructures===z.ParameterStructures.byPosition&&i.error(`Notification ${w.method} defines parameters by position but received parameters by name`),h(w.params)):f&&f(w.method,w.params)}catch(b){b.message?i.error(`Notification handler '${w.method}' failed with message: ${b.message}`):i.error(`Notification handler '${w.method}' failed unexpectedly.`)}else ne.fire(w)}function Rs(w){if(!w){i.error("Received empty message.");return}i.error(`Received message which is neither a response nor a notification message:
                                                                                                                                                                                                                              ^

Zu [Error]: Unhandled method client/registerCapability
    at Fs (/home/remco/Projects/vscode-remark/out/remark-language-server.js:25:223)
    at Cs (/home/remco/Projects/vscode-remark/out/remark-language-server.js:24:11203)
    at Immediate.<anonymous> (/home/remco/Projects/vscode-remark/out/remark-language-server.js:24:11056)
    at processImmediate (node:internal/timers:464:21) {
  code: -32601,
  data: undefined
}

Unfortunately I also couldn’t easily get source maps to work for debugging. Changing line 11 in src/extension.js to const args = [require.resolve('remark-language-server'), '--stdio'] helps with debugging.

It would also help if someone can manually confirm it works on macOS or Windows. Especially Windows development setups are pretty alien to me. Can people just invoke node?

I also tried using LSP over node-ipc instead of stdio earlier. This caused issues. I think it disallows to use ESM.

I currently have limited time to look into this, but I do really want to finish this!

@wooorm
Copy link
Member

wooorm commented Jan 27, 2022

ctx.asAbsolutePath seems to be in the right direction: https://github.com/angular/vscode-ng-language-service/blob/2deee1829a386f2f0981116378508b71ae974f3a/client/src/client.ts#L499-L514.

so ubuntu seems to work always, macOS is flakey, and windows fails always?

@remcohaszing
Copy link
Member Author

Kind of but not quite? I see an Ubuntu build failing while typing this comment.

I think using the ipc transport might also make it a bit more stable.

@wooorm
Copy link
Member

wooorm commented Jul 12, 2022

Can you mention me again when this is in need of a review?

@wooorm
Copy link
Member

wooorm commented Jul 13, 2022

We probably should also remove the lockfile, and clean other repo things, in further PRs, btw!

@remcohaszing
Copy link
Member Author

Omg all checks have passed! 🎉

I agree we can do some more cleanups in other PRs. Let’s focus on this one first though. I’ll fix various quirks after this PR has been merged.

@wooorm wooorm changed the title Use remark-language-server Rewrite to use remark-language-server Jul 13, 2022
@wooorm wooorm merged commit da00a11 into remarkjs:main Jul 13, 2022
@wooorm
Copy link
Member

wooorm commented Jul 13, 2022

Sweet, landed!
Let me know what your plans are, to first update some more things (?), and to release something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants