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

Allow JSON with comments in jsconfig.json #7426

Open
mrmckeb opened this issue Jul 25, 2019 · 13 comments · May be fixed by #8140
Open

Allow JSON with comments in jsconfig.json #7426

mrmckeb opened this issue Jul 25, 2019 · 13 comments · May be fixed by #8140

Comments

@mrmckeb
Copy link
Collaborator

@mrmckeb mrmckeb commented Jul 25, 2019

Is your proposal related to a problem?

This is a follow-on from #7248. Right now, we don't support JSONC (JSON with comments) in jsconfig.json files.

Describe the solution you'd like

After a discussion with @iansu, we see two paths:

  1. Implement the same solution as we did for TypeScript. This is easy, but would require us making TypeScript a dependency of react-scripts.
  2. The above, but instead of installing typescript as a dependency of react-scripts, we would install it to the user's project if they are using a jsconfig.json file and don't have typescript installed.

Discussion is welcome.

If you're interested in picking this up, please let us know.

@mrmckeb mrmckeb added this to the 3.2 milestone Jul 25, 2019
@mrmckeb mrmckeb removed this from the 3.2 milestone Jul 25, 2019
@mrmckeb mrmckeb added this to the 3.1 milestone Jul 25, 2019
@miraage
Copy link

@miraage miraage commented Jul 25, 2019

https://www.npmjs.com/package/json5 seems to be quite popular. Both .json and .json5 could be supported with their syntax (i.e. no jsonc/json5 in .json files)

@mrmckeb
Copy link
Collaborator Author

@mrmckeb mrmckeb commented Jul 25, 2019

@miraage, I personally feel that adding a dependency to read JSONC files would be a poorer solution than adding TS as a dependency... adding TS actually gives us some other benefits, like potentially pinning TS versions (so that they align with the linting configs, etc).

@lianapache
Copy link

@lianapache lianapache commented Jul 25, 2019

@mrmckeb, I would like to pick this up.
Also, I think that moving TypeScript as a dependency to react-scripts sound like a breaking change, so I would rather go with the second option. What do you think?

@mrmckeb
Copy link
Collaborator Author

@mrmckeb mrmckeb commented Jul 25, 2019

Great, thanks @lianapache!

You should take a look at how verifyTypeScriptSetup.js works and either extend that, or see what can be shared here :)

@lianapache
Copy link

@lianapache lianapache commented Jul 25, 2019

@mrmckeb, Sure 👍 Thanks for the tip!

@miraage
Copy link

@miraage miraage commented Jul 25, 2019

@mrmckeb we can pin TS version via react-dev-utils in that case, like cross-spawn and chalk.

@iansu iansu removed this from the 3.1 milestone Aug 7, 2019
@iansu iansu added this to the 3.2 milestone Aug 7, 2019
@pardyalbert
Copy link

@pardyalbert pardyalbert commented Aug 18, 2019

Weldon guys

@iansu iansu added this to To do in v3.3 via automation Oct 23, 2019
@esvyridov
Copy link
Contributor

@esvyridov esvyridov commented Nov 20, 2019

@mrmckeb What if we parse json file as string and remove all comments?

@mrmckeb
Copy link
Collaborator Author

@mrmckeb mrmckeb commented Nov 21, 2019

@esvyridov if you'd like to pick this up and give that a go, let me know.

@esvyridov
Copy link
Contributor

@esvyridov esvyridov commented Nov 21, 2019

@mrmckeb sure, I'll take this 👍

@ianschmitz ianschmitz removed this from the 3.3 milestone Dec 5, 2019
@ianschmitz ianschmitz added this to the 3.4 milestone Dec 5, 2019
@esvyridov
Copy link
Contributor

@esvyridov esvyridov commented Dec 5, 2019

@mrmckeb I think I would try to implement the second path. The plan is:

  1. Check if there are comments in jsconfig
  2. If yes then I'll inform user that we found a comment in jsconfig and we need to install typescript to devDependencies in order to parse it.
  3. If not then just parse it

@mrmckeb
Copy link
Collaborator Author

@mrmckeb mrmckeb commented Dec 9, 2019

@esvyridov, what if we did this?

  1. Check if TS is installed, and parse with that - or parse without if not installed.
  2. If we fail, and TS was not installed, tell the user that they need to install TypeScript as a dependency if they want to support JSONC.

@esvyridov esvyridov linked a pull request Dec 10, 2019 that will close this issue
@esvyridov
Copy link
Contributor

@esvyridov esvyridov commented Dec 11, 2019

@mrmckeb PR #8140 is ready

@iansu iansu removed this from the 3.4 milestone Feb 14, 2020
@iansu iansu added this to the 3.5 milestone Feb 14, 2020
@raix raix removed this from the 5.1 milestone Dec 17, 2021
@raix raix added this to the 5.0.1 milestone Dec 17, 2021
@iansu iansu removed this from the 5.0.1 milestone Apr 12, 2022
@iansu iansu added this to the 5.0.2 milestone Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v3.3
  
To do
Development

Successfully merging a pull request may close this issue.

8 participants