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

Replace json parser with jsonc. #1147

Merged
merged 4 commits into from
Feb 16, 2022
Merged

Replace json parser with jsonc. #1147

merged 4 commits into from
Feb 16, 2022

Conversation

joshpoll
Copy link
Contributor

This PR replaces the JSON parser with JSONC. JSONC is an extension of the JSON spec that includes comments and trailing commas. It's supported by Microsoft and used in Monaco and VSCode.

This should provide a significant quality-of-life improvement for people using the editor.

It's still possible to export and share JSON specs, and this is the default behavior. Users can opt-in to JSONC specs when sharing by clicking the "Preserve whitespace, comments, and trailing commas" button.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Neat. It's pretty clean overall.

@arvind do you have any comments on this pull request?

src/utils/monaco.ts Show resolved Hide resolved
@@ -5799,6 +5799,11 @@ json5@^2.1.2, json5@^2.2.0:
dependencies:
minimist "^1.2.5"

jsonc-parser@^3.0.0:
Copy link
Member

Choose a reason for hiding this comment

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

Does Monaco depend on some jsonc parser already that we could use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like Monaco depends on the same jsonc-parser package: https://github.com/microsoft/monaco-editor/blob/7826dee4ea80e7e4542db6934593e370f2305d97/package.json#L43. I think the jsonc-parser interface is a lot more straightforward than the Monaco interface, but it might be possible to wrap Monaco.

Copy link
Member

Choose a reason for hiding this comment

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

It's a dev dependency in Monaco.

I'm asking since I would have expected the dependency to already be in the lock file.

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 I see! yeah I didn't realize that

Copy link
Member

@arvind arvind left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for addressing this long-running feature request @joshpoll!! My one minor comment is whether we could use a more descriptive alias than parse for the jsonc import? There's a lot of parsing happening in the editor, so a more distinctive name would help readability/understandability when people come back to the code base.

@joshpoll
Copy link
Contributor Author

@arvind I changed the parse import names

@domoritz domoritz merged commit dfa71a1 into vega:master Feb 16, 2022
domoritz pushed a commit that referenced this pull request Feb 16, 2022
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.

None yet

3 participants