-
Notifications
You must be signed in to change notification settings - Fork 240
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
"strictest" includes potentially superfluous options #156
Comments
Strictest options are really options which are pedantically strict (and thus not really recommended for most people) not that its an issue with backwards compatibility. New flags and features get added to strict all the time - so realistically, we don't want to be pushing it as though it is actually an improvement in the way you phrase it.
|
Thanks for the quick reply orta.
I guess I just don't understand this sentence. How would you phrase it then? For me, it is 100% an improvement. To make clear what I mean:
In both of these cases, I am happy to upgrade. As an end-user, if I didn't want things to be maximally strict like this, then I would be inheriting from the "Recommended" config, not the "Strictest" config. And, as the OP requests, regardless of what we choose the point of the config to be, we should still definitely document it in the readme! |
+1 for the documentation part |
I would like to echo the other commenters here, As others have indicated, if you feel strongly that the compiler should not error on some things when using this config, then I think it would be good to document what exactly you think the purpose of this config is, since it doesn't seem to align with what we are expecting. My personal use case is that when starting a new project I want to start with all-the-errors, and over time I may choose to disable some but that is much easier than trying to add strictness later. |
Strictest being "recommended + things which the TS team think are strictness flags but are too pedantic for most people to be in strict" is always how I've worked with the tsconfig base and I think that definition still holds well after that time. That said, what no-one has explored in this thread is pulling out the idea of "recommend +" because in TS 5.0 we can now do multiple tsconfigs and we've already extracted that into its own base. So, instead we could semver major all the tsconfigs, removing the recommended fields and maybe make each include a dependency of |
My concern with this definition is it roughly translates to "what person X feels like, but stricter than recommended". Such a base config is not unreasonable to have (I'm not arguing against opinionated configs here), but I feel it should be named something that more clearly implies that the config is opinionated. The current name of "strictest" implies that it is the most strict compiler settings you could possibly have (the most possible errors/warnings). This is quite unlike "recommended", which very clearly implies it is an opinionated base config provided by the authors of this repository. In a feeble attempt to offer solutions rather than just problems, perhaps this config (current settings) could be renamed to |
I'm not the one making these decisions anymore, but when I was on the team those were the TS team decisions, not "person x": recommended comes from There are no "strictest" tsconfig options which are not pedantic, if it was a useful strict option which was recommended by the TS team - it would be in Thus "strictest" is not recommended, but it is stricter - calling it |
You make a good point about renaming being backward incompatible. How about a
As per the discussion in this thread, a "esModuleInterop": true,
"skipLibCheck": true, |
TBH - Those come from recommended, not from the pedantic flags, and would be removed if we split that out re: #156 (comment) |
Ah, I see what you are saying. If the switch to separate configs that you proposed in #156 (comment) were to occur, what would be the process for a user getting the strictest possible compiler settings (equivalent to current |
Yep, I'd be saying they should have If they wanted to not have those two fields which are set in recommended set, they can be changed to different values those in their app - just like you would today. The indication that you have put them in your own personal tsconfig indicates the conscious choice to override what the TS team considered recommended - which is totally fine (and kinda the point of bases) |
It sounds like the ship has sailed on renaming "esModuleInterop": false,
"skipLibCheck": false, |
In the docs, we could reverse the order so that |
Hello Orta! Thanks for maintaining these configs.
Document "Strictest"
The first problem I would like to raise is that the "strictest" config is not documented. I think that in the README.md file, we should explain what the point of the config is.
I'll give it a go, and feel free to copy paste this into the README:
Is this accurate?
Option Violations
Based on this definition, it would seem that the following options are in violation:
esModuleInterop
has to do with the target environment. From what I understand, it is not needed in all environments. And I suspect it could actually be harmful in certain situations. Thus, I think this should be taken out.skipLibCheck
skips type checking on declaration files. This is purely an optimization to speed up compilation. Turning it on is generally a good idea in big repositories that take a lot of time to compile. But turning it on notably makes the compiler less strict, not more strict. Thus, I think this should be taken out. More on this below.More Discussion on
skipLibCheck
In my type definitions library, I extend from "Strictest" in the same way that I do with all of my other TypeScript projects, without giving it a second thought. But having
skipLibCheck
in there is a hidden gotcha - it completely disabled type checking for the entire library, unbeknownst to me! Today, it caused me to ship an error to production.In general,
skipLibCheck
is a pretty good compiler option to have turned on, so I can totally see the motivation here - it kind of makes sense to pack together "optimization" related compiler options together with "type-checking" related compiler options. ButskipLibCheck
in particular is really dangerous, so I think that optimization related options should be kept out of the "Strictest" config!If you think removing
skipLibCheck
would be really harmful, then we should instead backfill that option into the individual target environment configs e.g.Next.js + Strictest
,Electron + Strictest
, and so on.The text was updated successfully, but these errors were encountered: