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

next dev rewrites tsconfig.json #8128

Closed
borekb opened this issue Jul 26, 2019 · 16 comments · Fixed by #13458
Closed

next dev rewrites tsconfig.json #8128

borekb opened this issue Jul 26, 2019 · 16 comments · Fixed by #13458
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@borekb
Copy link
Contributor

borekb commented Jul 26, 2019

Bug report

Describe the bug

When I run next dev and some of the "required" Next.js options are missing (even non-critical ones like forceConsistentCasingInFileNames), my tsconfig.json is completely rewritten – my comments are deleted and the file is re-formatted.

To Reproduce

  1. Start next dev
  2. In tsconfig.json, remove forceConsistentCasingInFileNames and add some comment.
  3. Start next dev again
  4. Observe that your comment is deleted

Expected behavior

My tsconfig.json is my tsconfig.json 😄.

System information

  • OS: macOS
  • Version of Next.js: 9.0.2

Additional context

This is from the "TypeScript support is too eager in v9" bucket, IMO. Some more issues I'd regard in that category are e.g. #7687 or #8065 (though they are about different things).

@Janpot
Copy link
Contributor

Janpot commented Jul 26, 2019

Also #8034

@Timer
Copy link
Member

Timer commented Jul 26, 2019

If you don't want forceConsistentCasingInFileNames to be true, set it to false instead of deleting the field.

We can look into retaining comments.

@borekb
Copy link
Contributor Author

borekb commented Jul 28, 2019

It's indeed the same issue as #8034, sorry for not noticing it earlier.

It's OK for Next.js to generate new tsconfig.json when there is none (it's very useful, actually) but overwriting existing files, possibly losing user's work, is something that no tool should do, ever, IMO.

It's also debatable whether Next.js "needs" some of the things it says so, for example, I don't think compilerOptions.isolatedModules is required for Babel and there are serious performance implications of having this turned on, for example, microsoft/TypeScript#32294.

What I'm trying to say: in the end, tsconfig should be user's responsibility. It's fine if Next.js makes reasonable suggestions or does some changes automatically in some cases, but it should be careful and conservative, IMO.

@Timer
Copy link
Member

Timer commented Jul 29, 2019

@borekb Whether or not compilerOptions.isolatedModules is needed is not debatable -- it is an invariant.

Without this option, TypeScript features that are not supported by Babel will break in unexpected / indeterminate ways and cause countless hours of debugging.

I agree that the config overwrite should never be "destructive", and we'll work on fixing this.

@borekb
Copy link
Contributor Author

borekb commented Jul 29, 2019

isolatedModules is certainly debatable – for example, we'd like to make a tradeoff between performance and safety in our case. (Next+TS can run perfectly fine without this option.) It's hard for a tool to predict all the possible circumstances and be 100% right all the time.

I just don't think that Next.js should be writing into my source files, especially as a by-product of something as innocent-looking as next dev. I'd prefer any of these:

  • Next.js could encode the default tsconfig in some internal way, similarly to how webpack config is hidden from me
  • next dev could validate that the configuration is correct and warn me / refuse to start if some critical config is not met
  • There could be a manual command like next init-tsconfig

Maybe there are some other options as well. I understand the good intent though and appreciate your effort around this!

@Timer
Copy link
Member

Timer commented Sep 17, 2019

I've scheduled this for work in 9.0.7, here's what we'll do:

  1. Next.js will not erase comments from your tsconfig.json.
  2. Next.js will not override options you have set without prompting you first. You can answer "No" and edit them manually.
  3. Next.js will still create a default (but overridable) tsconfig.json for the user if not present.

@Timer Timer added this to the 9.0.7 milestone Sep 17, 2019
@majelbstoat
Copy link

That's great, thank you 🙏🏼 Can we please also support its (and next-env.dts') location in a configurable place?

@webhacking
Copy link

9.0.7-canary.4 works fine.

@jamesreggio
Copy link
Contributor

jamesreggio commented Oct 17, 2019

@Timer I'm a little unclear of your plan from this comment: #8128 (comment)

If I've specified "isolatedModules": false, will it be possible to run next dev without it being overwritten?

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels Oct 18, 2019
@Timer Timer modified the milestones: 9.1.2, 9.1.3 Oct 23, 2019
@Timer Timer modified the milestones: 9.1.3, 9.1.4 Nov 8, 2019
@Timer Timer modified the milestones: 9.1.4, 9.1.5 Nov 19, 2019
@Timer Timer modified the milestones: 9.1.5, 9.1.6 Dec 10, 2019
@ijjk ijjk removed this from the 9.1.6 milestone Dec 17, 2019
@ijjk ijjk added this to the 9.2.0 milestone Dec 17, 2019
@HelloMyDevWorld
Copy link

Version 9.1.6 still override everything...

@timneutkens
Copy link
Member

timneutkens commented Dec 20, 2019

So feel free to send a PR, the issue has “good first issue” and “help wanted” labels.

Here’s what you can implement:
#8128 (comment)

@Timer Timer modified the milestones: 9.2.0, 9.2.x Jan 3, 2020
@stvmachine
Copy link

Yep, 9.1.6 still override.

@timneutkens
Copy link
Member

timneutkens commented Mar 26, 2020

@stvmachine I'm not sure what you're trying to achieve with this comment. You contribute this feature as it's marked as good first issue and help wanted as said before.

@kodiakhq kodiakhq bot closed this as completed in #13458 May 27, 2020
kodiakhq bot pushed a commit that referenced this issue May 27, 2020
This pull request updates our TypeScript verification process to not wipe out potentially vital user comments.

Introducing a prompt process was mostly a side effect of users wanting to keep comments.
There's no reason we really need this prompt, as answering no would refuse to boot the Next.js server anyway.

---

Fixes #8128
Closes #11440
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
This pull request updates our TypeScript verification process to not wipe out potentially vital user comments.

Introducing a prompt process was mostly a side effect of users wanting to keep comments.
There's no reason we really need this prompt, as answering no would refuse to boot the Next.js server anyway.

---

Fixes vercel#8128
Closes vercel#11440
@rikkit
Copy link

rikkit commented Oct 27, 2020

Hey there,

For better or worse, I am a user of ambient const enums. We have a "legacy codebase" and the effort to refactor away from using them has been judged as far too much - we just have too much code that relies on their existence. We have written a Babel plugin to allow Babel to compile code that uses these enums. This is working well with our Storybook project, which is another project that relies on Babel for TypeScript transpilation - from this we know that Babel works just fine with isolatedModules: false.

The remaining hurdle for deploying our new NextJS project is that tsconfig.json must have isolatedModules set to false when running next build. Like @borekb writes above, I appreciate the intention of the NextJS team here, but I would like a way to opt-out of this behaviour without completely turning off TypeScript checking. Babel itself does not forbid this option, so I am not sure that NextJS should enforce it so rigorously.

This issue appears to have been closed automatically as NextJS no longer removes comments, but as of 9.5.3 the isolatedModules option is still mandatory.

Would the team accept a PR that allowed next build to run with isolatedModules: false? I am thinking of a config option ignoreIsolatedModulesError that matches the existing ignoreBuildErrors option which disables type checking completely.

@rschristian
Copy link

This is quite hacky and awful, feel free to improve, but here's post-install script that may be useful for anyone else who really isn't a fan of their config files being overwritten:

https://gist.github.com/rschristian/7051745e06e235381924291f9285d938

@TroyAlford
Copy link

For anyone else struggling with this - I applied @rschristian's solution using patch-package. This makes it automatically re-apply without any extra plumbing, in a super consistent way.

Steps:

  • install patch-package (and postinstall-postinstall if you're using yarn)
  • make the edits shown in the above gist from @rschristian directly within your node_modules/next folder
  • run yarn patch-package to create the .patch file
  • in package.json add a script that does "postinstall": "patch-package"

Voila, every time you add/remove/update your npm packages, the patch will automatically be re-applied. And patch-package is also smart enough to warn you if your next version upgrades, because you might need to update your patch when that happens.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet