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

Include typings from subdirectories #11

Closed
wants to merge 1 commit into from

Conversation

adamvoss
Copy link
Contributor

@adamvoss adamvoss commented Aug 27, 2017

Removes an .npmignore entry that prevents some typing information from being published.


Rational:

vscode-yaml-languageservice (powers vscode-yaml) reuses the AST representation from this library to build an AST from a YAML document, and it feeds that representation to services from this library to gain most of the validation and other goodies from this library.

To overcome the lack of published type definitions, vscode-yaml-languageservice used git submodule to depend directly on the source code from this repository. While this worked it was inconvenient and fragile (broken by changes in npm behavior).

I would really like a better story for managing the vscode-json-languageservice dependency that could preferably use npm like usual. This is the easiest solution, requiring the least additional management I have come up with. I recognize that, even if type information is published, vscode-yaml-languageservices is depending on "internal" APIs and breaking changes could occur at any time.

(This has been sparked somewhat by an increased interest in the YAML support provided by vscode-yaml [on private channels] and possibly wanting to use the service to add support in other editors).

@aeschli
Copy link
Contributor

aeschli commented Sep 1, 2017

I guess what you really want is API. Can you describe what API's you need?
ASTNodes, what APIs on it? Just node ranges, or validate?
SchemaService?

@adamvoss
Copy link
Contributor Author

adamvoss commented Sep 3, 2017

From a design perspective that would be the "right" way to do things. It would certainly be am more involved change though.

If this is the route you want to go I can seem what I can come up with. It is a little hard because I am trying to do the exact same thing as this service only swapping out the language being processed which means I am using more of the interface than not; however, I see no reason a balance could not be found.

At the moment the best information I can provide is probably my experience implementing YAML for each of the main functions.

Validation

This is really the minimum expectation for alternative language implementation, so the least that must be supported.

Replacing the parser that builds the AST provides the provides document validation and error highlighting, the services themselves have no required modification.

^ YAML is a little more complicated here since there can be multiple documents in one file.

Completion

I will know more once I implement this for YAML. I think the service as-is will generally need to be replaced entirely. However, experience will show me if there are parts of the service that remain useful.

Hover

Members of JSONDocument related to getting a node from an offset needed to be overwritten.

findDocumentSymbols

So far this has just worked without requiring any modifications.

Format

I also group this with completion as not reusable.

Colors

I have not yet taken the time to make this work so cannot comment on it other than saying the old color system did not "just work" and would have required modification. Trying the new changes for colors has been somewhat impeded by it being a preview feature and source releases trailing npm releases.

@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@aeschli
Copy link
Contributor

aeschli commented Apr 26, 2018

With the latest API you can now create a JSONDocument from an ASTNode.
The ASTNodes have been added to the offcial API.

@aeschli aeschli closed this Apr 26, 2018
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.

4 participants