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

module: allow JSONC imports #52699

Closed
wants to merge 1 commit into from
Closed

Conversation

avivkeller
Copy link
Member

This PR introduces the ability to import JSONC typed JSON files via the { type: 'jsonc' } import attribute.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Apr 26, 2024
@avivkeller
Copy link
Member Author

When I get a change, Ill adjust the small prettification differences

@GeoffreyBooth
Copy link
Member

Do any other runtimes support JSONC or plan to?

@avivkeller
Copy link
Member Author

avivkeller commented Apr 26, 2024

I know Deno supports it with its builtin standard library (https://deno.land/[email protected]/jsonc), but I'm not sure about importing

I'll need to check the other runtimes, and more about Denos imports

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Blocking for the following reasons:

  1. There's been no discussion of whether this is a good idea at all, much less something worth adding to core; and whether the best approach is via extending import as opposed to other alternatives such as a utility function.

  2. Importing JSON is supported by browsers and there is a spec around it. I'm unaware of anything similar for JSONC. We shouldn't add import abilities for unspecified types. That is better handled by userland hooks.

@JakobJingleheimer
Copy link
Member

  1. That is better handled by userland hooks.

If #49704 ever gets revived, that could also facilitate this so users don't have to manually sort out something almost everyone wants.

// Some comment
/* Also
a comment */
}
Copy link
Member

Choose a reason for hiding this comment

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

nit... looks like some linting issues with these files (among other things there should be a new-line at the end.

@avivkeller avivkeller force-pushed the jsonc-imports branch 2 times, most recently from 328ccd3 to caa5897 Compare April 27, 2024 17:36
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I don't think this is a good feature to add either.

Additionally, the approach to strip comments from jsonc files is incorrect, check what userland modules do instead. Regular languages and all that..

@avivkeller avivkeller closed this Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants