unified-language-server

unified-language-server copied to clipboard

Initial checklist

• [X] I read the support docs
• [X] I read the contributing guide
• [X] I agree to follow the code of conduct
• [X] I searched issues and couldn’t find anything (or linked relevant results below)

Problem

There are several reasons the unified engine throws an error. It’s currently not tested though: https://github.com/unifiedjs/unified-language-server/blob/f222a6552e04c50c4bf6933b0539898ece9c47ff/lib/index.js#L173-L178

Solution

Add a test

Alternatives

n/a

Another option would be adding support for checking the schema with a validator to give even better messages. @remcohaszing contributed a few in https://github.com/SchemaStore/schemastore/pull/1724 and https://github.com/SchemaStore/schemastore/pull/1982 which could be leveraged

This issue was particularly about missing unit tests. A comment in the code says something cannot occur, but it can, and should be tested. Some examples: a plugin throwing an error, a broken JavaScript file somewhere. I don’t see how schemas solve these problems. I’m not sure what problem you want to address with schemas?

I don’t see how schemas solve these problems. I’m not sure what problem you want to address with schemas?

A schema would specifically address the issue mentioned in the title, testing for a broken configuration file.

The several examples you mention:

Some examples: a plugin throwing an error, a broken JavaScript file somewhere.

are all good things to account for, but aren't exactly a broken config?

This issue was particularly about missing unit tests. A comment in the code says something cannot occur, but it can, and should be tested.

Thanks for the clarification

testing for a broken configuration file.

You mean broken remark or rehype config files? In that case, I do not yet understand the pros and cons of what you propose and how it differs from the current state (where broken config files result in errors), and how it would work in unified-language-server (or unified-engine? Or somewhere else?)

but aren't exactly a broken config?

While the title is short, it is expanded upon in the problem description. Certain errors are thrown when unified-engine / unified-language-server are misconfigured, such as with missing reporters or when no input is given. These errors are real, and can be tested. The code currently says they never occur, and doesn’t test them.

You mean broken remark or rehype config files?

I do

I do not yet understand the pros and cons of what you propose and how it differs from the current state

The main difference would be (for json and yaml configurations), we may be able to provide more detailed messages for invalid configuration, at the cost of including a json schema library

how it would work in unified-language-server (or unified-engine? Or somewhere else?)

It would be a validation step that would happen after configuration file is loaded, and before we attempt to interpret/run the configuration.

These errors are real, and can be tested. The code currently says they never occur, and doesn’t test them.

Gotcha, thanks for the clarification

I think supporting JSON schema based validations might be interesting. I personally have some good experience using jsonschema and yaml to point users to the exact file and position in a file that’s invalid. I think this is also possible using jsonc-parser.

ESLint also uses JSON schemas to validate plugin options. It could be done for unified plugins. It should be optional though, since many existing plugins don’t use it.

However, I don’t think this is what the issue is about. It’s about testing how unified-language-server deals with broken configuration files.

It might be useful but I’m not sure how much schema validation in the engine/unified-language-server adds compared to:

• existing types
• existing runtime errors when unexpected values are passed in plugins

Something prop-types/ESLint like feels verbose to me, also ’cause we already have the above 2.

But improving error messages/traces for broken/unexpected values in general are :+1: for me. Not sure how to do it for JS config files? Perhaps ASTs even? More likely something with https://github.com/felixge/node-stack-trace / https://v8.dev/docs/stack-trace-api? Or not at all. Generally also :+1: on adding tools to improve things to the engine (similar to editorconfig), but very cautious to add it to unified itself.

As an aside, more for @remcohaszing, settings in remarkrc/rehyperc in SchemaStore as you added them are likely useful, but not strictly correct (as anything can be passed in settings, and any plugin could read from them and expect whatever, and the default *parse and *stringify plugins could be swapped for something else). There’s also the uncommon case where remark -> rehype would be done in a config file. Not sure how to handle that.

I think we’ve gone a bit off track with the discussion. The original issue is not about any form of schema validation, but about:

1. Missing test coverage.
2. Adding a test for handling broken coverage.

I added a test, but it doesn’t hit those lines. unified-engine handles a broken config gracefully. So I don’t really think I need to push this, but I can if you want. You can see this easily by creating a file named .remarkrc.json in a repo with the contents {. Then open a markdown file with VSCode and vscode-remark installed.

I don’t know what does cause unified-engine to throw.

incorrect options directly passed to engine yields an error, such as: https://github.com/unifiedjs/unified-engine/blob/8fd1e22bd38be8a55bb2c3f70fa308cbc9d226a8/lib/index.js#L360.

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.