-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add support for JSONC project configuration files #13
Conversation
Sick Let me test this on my repos. If it works, I'll merge asap. |
I've spent about an hour+ trying to get this change to work in my local, and it just won't do it. Give me more time and I'll def try and figure out why. Unsure how you're testing, but I'm doing Edit: this is bugging the shit out of me. |
@jonkwheeler Hmm, this might be my fault: I used NPM (instead of Yarn) to install stuff, and I did notice it touched the Btw, I'm testing by installing directly from GitHub: |
Currently using that fork in this monorepo which has a |
hello, JSON.parse(
readFileSync(file).toString('utf8')
.replace(/\/\*.*?\*\//g, '')
.replace(/\/\/.*/g, '')
.replace(/\/\*(.|\n)*?\*\//g, '')
) as IRawTSConfig; It worked with my TSconfig even when adding comments like so it should be the same result as the one from the dependency : {
"t": /* hello */ true,
/*
* really big
* comment
*/
} Here are a tsconfig and the output of those regex replace. |
@vic1707 Personally, I prefer the battle-tested dependency (33,000,000 weekly downloads) over an adhoc solution. Plus, looking at |
Understandable, I was just proposing the regex solution in case @jonkwheeler wasn't able to solve his issue 😉. Also because I prefer to rely the least on external dependencies (maybe not the Best practice ? Feel free to tell me I'm quite new to JS/TS outside of ANGULAR) As for error positions (I'm assuming that an error could be thrown while parsing the JSON) I replaced all the whitespaces from the transformed file myself. I wanted to show a more concise file (the three replaces from the code leave all the space taken by the comment blank). |
Just to chime in, this morning I even copy pasta'd the file from |
I'm usually of the same opinion, but when I'm dealing exclusively with tooling (rather than something that's bundled and pushed to the browser), I don't mind extra deps 😄
No rush! Thanks for your work on this. |
See: #15 |
Just had a PR Party. You guys can go nuts now. |
Uses
strip-json-comments
to load TSConfig files instead ofrequire
. There are no unit tests, but it works across my repos (including my monorepos).Closes #10