-
Notifications
You must be signed in to change notification settings - Fork 45
feat: classify jsonc and json5 modules #627
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
Conversation
| // todo(#239): remove this when updating the --json output for 3.0 | ||
| #[serde(rename = "asserted")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now might be the time to address this? @dsherret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to move this code out of deno_graph and into the cli crate so it can be made stable there and then we can start doing more drastic changes to deno_graph.
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Maybe we should only support json5 and not bother with having an additional jsonc attribute though?
Also, it would be nice if we changed deno_graph to not load json modules and instead marked them as external similar to bytes and text imports.
| media_type: MediaType::Json5, | ||
| mtime: opts.mtime, | ||
| source, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could reduce duplication with the above? For example, something like resolving a json_media_type with Option<MediaType> then return this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe after JSON is changed from assert to coerce semantics like these ones, it's not nicely dedupable yet
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add an unstable option for this:
Lines 1655 to 1657 in decafab
| pub unstable_bytes_imports: bool, | |
| /// Support unstable text imports. | |
| pub unstable_text_imports: bool, |
| /// Support unstable jsonc and json5 imports. | ||
| pub unstable_extended_json_imports: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead have separate options for unstable_jsonc_imports/unstable_json5_imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just have json5 imports since json5 is a superset of jsonc
jsonc/json5deno#20374This generalises JSON module slots to any 'independent' or dependency-less text modules.