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

feat(formats): add support for Microsoft's JSONC format #732

Merged

Conversation

TrevorBurnham
Copy link
Contributor

Issue #, if available: #698

Description of changes: This PR adds full support for JSONC (specifically the implementation used by VS Code for its configuration files) as a configuration and token format.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. ✅

@@ -79,6 +79,7 @@
],
"transform": {
"^.+\\.json5$": "json5-jest",
"^.+\\.jsonc$": "json5-jest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I've taken advantage of the fact that JSON5 is a superset of JSONC, so the same transform satisfies Jest. I initially writing my own JSONC transform, based on the JSON5 one—it's basically a one-liner—but for some reason I couldn't get Jest to actually run it. 🤷‍♂️

Copy link
Collaborator

@chazzmoney chazzmoney left a comment

Choose a reason for hiding this comment

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

This is some really solid work.

LGTM! :shipit:

Thanks @TrevorBurnham

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -228,6 +228,11 @@ describe('extend', () => {
expect(StyleDictionaryExtended).toHaveProperty('platforms.web');
});

it('should accept a string as a path to a JSONC file', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nice unit tests!

@@ -12,6 +12,7 @@
*/

require('json5/lib/register');
require.extensions[".jsonc"] = require("./utils/jsonc").register;
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@chazzmoney chazzmoney merged commit cfa83cb into amzn:main Nov 9, 2021
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.

3 participants