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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
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
@RedYetiDev
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?

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Apr 26, 2024

I know Deno supports it with its builtin standard library (https://deno.land/std@0.223.0/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
Contributor

  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.

doc/api/esm.md Outdated Show resolved Hide resolved
// 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.

@RedYetiDev RedYetiDev 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..

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.

None yet

6 participants