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

Expose JSON worker #4299

Merged
merged 5 commits into from
Jan 8, 2024
Merged

Expose JSON worker #4299

merged 5 commits into from
Jan 8, 2024

Conversation

mevisioam
Copy link
Contributor

@mevisioam mevisioam commented Dec 13, 2023

This PR fixes #727 and addresses review comments (I think so) from an original PR #3956 which became stale for some reason.

I just want to get things JSON worker faster to Monaco, because it's needed for my project.

My use case is to provide insights to the end user after analyzing JSON schemas of the edited document. Plus, include the motivation from an original PR:

Exposes the JSON worker and the parseDocument function from the JSON Worker.

This is useful when editors want to use the JSON Worker's ability to parse the document, as using a separate parser can result in inconsistencies and does double the work.

@mevisioam
Copy link
Contributor Author

@microsoft-github-policy-service agree

@mevisioam
Copy link
Contributor Author

@hediet @aeschli can you take a look? Since you looked at #3956 previously :)

@@ -4,7 +4,8 @@
*--------------------------------------------------------------------------------------------*/

import * as mode from './jsonMode';
import { Emitter, IEvent, languages } from '../../fillers/monaco-editor-core';
import { Emitter, IEvent, languages, Uri } from 'monaco-editor-core';
import * as jsonService from 'vscode-json-languageservice';
Copy link
Contributor

Choose a reason for hiding this comment

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

The types that are used from jsonService should be copied to json/monaco.contribution.ts so that the d.ts file is self-contained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, lemme fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, I wonder then why we have pretty much the same setup in https://github.com/microsoft/monaco-editor/blob/main/src/language/typescript/monaco.contribution.ts#L6 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, it's jsonMode not jsonService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aeschli looks crazy, but I'm done, sir :)

@aeschli
Copy link
Contributor

aeschli commented Dec 18, 2023

Yes, that's a lot of types, I don't like it either. Especially as these types duplicate some of the Monaco types (e.g. IPosition, IRange), but Monaco types 1 based (the first column and line is 1, not 0), while the LSP types are 0 based.

Not sure what approach to choose:

  • Keep the API minimal. What API from the worker do you really need?
  • In typesscript/monaco.contribution.ts the APis just return any. We could do that.

@alexdima What do yo think?

@mevisioam
Copy link
Contributor Author

@aeschli:

  • Keep the API minimal. What API from the worker do you really need?

Think, I could leave it to only getMatchingSchemas and parseDocument as in the initial PR did, afair.

@mevisioam
Copy link
Contributor Author

mevisioam commented Dec 20, 2023

@aeschli cleaned up a lil bit

src/language/json/monaco.contribution.ts Outdated Show resolved Hide resolved
@mevisioam
Copy link
Contributor Author

@aeschli @alexdima this state shud be fine now.

@aeschli
Copy link
Contributor

aeschli commented Dec 20, 2023

Looks good!

@aeschli aeschli added this to the December / January 2024 milestone Dec 20, 2023
@mevisioam
Copy link
Contributor Author

@aeschli thx for the review, when shall this end up in npm install monaco-editor? :)

@aeschli aeschli merged commit 698e0ec into microsoft:main Jan 8, 2024
3 checks passed
@aeschli
Copy link
Contributor

aeschli commented Jan 8, 2024

I think @hediet makes a Monaco editor release in the same week VS Code is released. So this should be in the last week of January.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose access to JSON worker
4 participants