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.

Sign up for GitHub

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

Jump to bottom

toSourceCode of JSON files should support JSONC #654

Closed
charlespwd opened this issue Dec 5, 2024 · 2 comments · Fixed by #656
Closed

toSourceCode of JSON files should support JSONC #654

charlespwd opened this issue Dec 5, 2024 · 2 comments · Fixed by #656

Comments

@charlespwd
Copy link
Contributor

charlespwd commented Dec 5, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.
We need a jsonc parser -> json-to-ast#JSONNode adapter.

Linting JSONC files doesn't do anything because json-to-ast throws an error when it sees a comment or a trailing comma.

Tried to do #547 but couldn't because the template files in horizon have the JSONC comment at the top and the sourceCode.ast was instanceof Error

https://github.com/Shopify/theme-tools/blob/main/packages/theme-check-common/src/to-source-code.ts#L2-L22

^ json-to-ast doesn't support JSONC. We need something that parses JSONC into an ast and transforms it into a JSONNode.
@charlespwd charlespwd mentioned this issue Dec 5, 2024
Merged
6 tasks
@charlespwd
Copy link
Contributor Author

Couple of options:

  • an adapter (fastest)
  • a fork of json-to-ast (faster) (it's MIT)
  • a pr to json-to-ast (slowest... probably)

@charlespwd
Copy link
Contributor Author

Notes:

  • json-to-ast hasn't seen a commit in 7+ years
  • author doesn't seem active on github either
  • 2 open PRs for 2+years

charlespwd added a commit that referenced this issue Dec 6, 2024
@charlespwd
Add AST support of JSONC (for theme check, language features, etc.)
57db721 
- Rip out `json-to-ast`
- Inline the `json-to-ast` types in `theme-check-common/src/jsonc`
- Make a little `jsonc-parser#Node` -> `json-to-ast#ASTNode` adapter

Fixes #654
@charlespwd charlespwd mentioned this issue Dec 6, 2024
Merged
1 task
charlespwd added a commit that referenced this issue Dec 6, 2024
@charlespwd
Add AST support of JSONC (for theme check, language features, etc.)
7d47a8d 
- Rip out `json-to-ast`
- Inline the `json-to-ast` types in `theme-check-common/src/jsonc`
- Make a little `jsonc-parser#Node` -> `json-to-ast#ASTNode` adapter

Fixes #654
charlespwd added a commit that referenced this issue Dec 6, 2024
@charlespwd
Add AST support of JSONC (for theme check, language features, etc.)
c371b48 
- Rip out `json-to-ast`
- Inline the `json-to-ast` types in `theme-check-common/src/jsonc`
- Make a little `jsonc-parser#Node` -> `json-to-ast#ASTNode` adapter

Fixes #654
albchu pushed a commit that referenced this issue Dec 9, 2024
@charlespwd @albchu
Add AST support of JSONC (for theme check, language features, etc.)
d01e657 
- Rip out `json-to-ast`
- Inline the `json-to-ast` types in `theme-check-common/src/jsonc`
- Make a little `jsonc-parser#Node` -> `json-to-ast#ASTNode` adapter

Fixes #654
navdeep5 pushed a commit that referenced this issue Dec 16, 2024
@charlespwd @navdeep5
Add AST support of JSONC (for theme check, language features, etc.)
76ea176 
- Rip out `json-to-ast`
- Inline the `json-to-ast` types in `theme-check-common/src/jsonc`
- Make a little `jsonc-parser#Node` -> `json-to-ast#ASTNode` adapter

Fixes #654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add AST support of JSONC (for theme check, language features, etc.) Shopify/theme-tools
1 participant
@charlespwd