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

refactor: made adding a new loader easier #155

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KilianKilmister
Copy link

No description provided.

@KilianKilmister KilianKilmister marked this pull request as draft July 12, 2020 20:57
@KilianKilmister
Copy link
Author

referencing my comment in #133

turned out to be a very small change.

  • js/cjs are loaded with dynamic import
  • other ext are mapped against an object 0f {[exst: string] (src: string) => object} (which currently just contains '.json': JSON.parse)
  • if ext doesn't have a mapping it falls back to the JSON.parse

@toddbluhm
Copy link
Owner

This is a very cool PR. I like the simplicity of it + the clarity it adds. Would make it a lot easier to support more file extensions.

@KilianKilmister
Copy link
Author

@toddbluhm awesome-sauce, I believe the tests only failed because i didn't pre-lint, so thats easily fixable. Now it's been a while since i wrote this and I'm still dealing with the aftermath of a corrupted system-drive on my main machine, so rewisiting the code won't be ultra swift, but i'll take your response as a green-light on polishing this up.

Want me to include YAML and/or JSON5 suport rightaway aswell? I'd suggest to include both, but it's your call.

@k-yle
Copy link

k-yle commented Sep 11, 2020

Any update on this? I'm happy to help get this merged if required. We would really like to use json with comments.

@toddbluhm if you're concerned about adding another dependency you could make it a peerDependency and/or just have a helpful error message if the user tries to use jsonc without that dependency installed. I'm happy to make a PR for this. facebook's create-react-app uses this approach

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.

None yet

3 participants