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

fix(core): move generator should work if there are comments in tsconfig #13740 #13866

Merged

Conversation

Coly010
Copy link
Contributor

@Coly010 Coly010 commented Dec 16, 2022

Current Behavior

If there are comments in the tsconfig, the move generator will fall over right away.

Expected Behavior

It shouldn't fall over if there are comments in the base tsconfig

Notes

Comments will be removed from the tsconfig file, but there is no good way to keep them there.

Related Issue(s)

Fixes #13740

@Coly010 Coly010 self-assigned this Dec 16, 2022
@vercel
Copy link

vercel bot commented Dec 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nx-dev ✅ Ready (Inspect) Visit Preview Dec 16, 2022 at 0:53AM (UTC)

@isaacplmann
Copy link
Collaborator

The jsonc parser is significantly slower than JSON.parse. Nx Console used to use jsonc parser everywhere and it was basically unusable in large workspaces. The base tsconfig is unlikely to be large enough for the performance difference to matter, but this can't be the solution for every JSON file.

@jdpearce
Copy link
Collaborator

Why are we doing this? JSON doesn't support comments and anyone who adds them should be doing so at their own risk.

@Coly010
Copy link
Contributor Author

Coly010 commented Dec 16, 2022

@isaacplmann This is only going to be used in one file during the move generator command. The tsconfig file. I should also note that readJson from @nrwl/devkit also uses jsonc-parser, granted it is a fallback if JSON.parse fails. I could do the same here.

@jdpearce Adding this here so that the context remains in the PR:

Angular CLI generates a tsconfig with comments.
Anyone using angular cli then using nx init will have trouble with the move generator.
image

I'll update the PR to only use jsonc-parser if the initial JSON.parse fails.

Copy link
Collaborator

@isaacplmann isaacplmann left a comment

Choose a reason for hiding this comment

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

Using jsonc-parser as a fallback sounds good to me. Especially if that's what devkit is already doing.

@Coly010 Coly010 merged commit 4f98e37 into nrwl:master Dec 16, 2022
@Coly010 Coly010 deleted the core/move-generator-support-json-comments branch December 16, 2022 14:20
FrozenPandaz pushed a commit that referenced this pull request Dec 19, 2022
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@nrwl/workspace:move fails on tsconfig.json with comments
3 participants