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.

Sign up for GitHub

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

Jump to bottom

Support JSONC format for jsconfig.json #8140

Closed
wants to merge 2 commits into from
Closed

Support JSONC format for jsconfig.json #8140

wants to merge 2 commits into from

Conversation

esvyridov
Copy link
Contributor

@esvyridov esvyridov commented Dec 10, 2019 •
edited by mrmckeb

Closes #7426.

To verify my changes I've used yarn create-react-app my-app inside create-react-app. Then I created jsconfig.json file with comments and run yarn start. I saw this error:
image

After I run yarn add typescript the error is gone.

Note: I've noticed a problem in getModules. Please check comments

@esvyridov
Copy link
Contributor Author

Probably I missed something but as I can see now we assume that typescript is installed based on appTsConfig (tsconfig.json). Is it ok?

@esvyridov esvyridov mentioned this pull request Dec 11, 2019
Open
mrmckeb
mrmckeb suggested changes Dec 15, 2019
View reviewed changes
Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

Thanks @esvyridov! This looks good overall, but I'd be interested in getting some feedback from another team member.

packages/react-scripts/config/modules.js Show resolved Hide resolved
packages/react-scripts/config/modules.js
@@ -130,7 +134,39 @@ function getModules() {
// Otherwise we'll check if there is jsconfig.json
// for non TS projects.
} else if (hasJsConfig) {
config = require(paths.appJsConfig);
if (hasTsSetup) {
const ts = require(resolve.sync('typescript', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using hasTsSetup, I think you could just do a try/catch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@mrmckeb mrmckeb added this to the 3.4 milestone Dec 18, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

I've tagged this for 3.4, and will ask another team member to review.

@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@flying-sheep
Copy link

Can this be merged please? I’ve just

  1. been bitten by the supremely unhelpful SyntaxError: .../jsconfig.json: Unexpected token / in JSON at position 170
  2. found a locked issue marked as duplicate of a PR that fixed this for tsconfig.json only (also locked)
  3. found this

It’s really hard to find this with all the issue locking

@flying-sheep
Copy link

Can I circumvent this bug in some way until this is merged? Tell CRA to ignore the file? I don’t want to make my config files worse by deleting perfectly fine comments.

@julianCast
Copy link

Hi! Thanks for the work!

What is the current state for this PR? It'd be really useful to be able to have nice comments on our json files.

@esvyridov esvyridov closed this by deleting the head repository Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrmckeb mrmckeb mrmckeb requested changes

@ianschmitz ianschmitz Awaiting requested review from ianschmitz

@iansu iansu Awaiting requested review from iansu

@petetnt petetnt Awaiting requested review from petetnt

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

Allow JSON with comments in jsconfig.json
6 participants
@esvyridov @mrmckeb @flying-sheep @julianCast @iansu @facebook-github-bot