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

"strictest" includes potentially superfluous options #156

Open
Zamiell opened this issue Feb 23, 2023 · 13 comments
Open

"strictest" includes potentially superfluous options #156

Zamiell opened this issue Feb 23, 2023 · 13 comments

Comments

@Zamiell
Copy link

Zamiell commented Feb 23, 2023

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:

In the TypeScript ecosystem, it is considered best practice to turn on the strict compiler flag so that the TypeScript compiler can catch as many bugs as possible. But counterintuitively, turning on the strict flag does not actually make the compiler as strict as possible. This is because the strict flag was created in April 2017 and it has been kept the same way for backwards compatibility. But there have been many other improvements to TypeScript other the years that are not included in the strict flag. Thus, if we want to leverage TypeScript to the fullest, we have to manually specify all of the new compiler flags one by one.

This is where the "Strictest" config comes in. It automatically enables every TypeScript compiler flag that has to do with type checking, making it as strict as possible. As TypeScript continues to improve, new flags will automatically be added to this config, freeing you from the tedium of having to keep up with the TypeScript patch notes.

Since the "Strictest" config only contains type-checking related flags and not any flags relating to the current environment or build output, the strictest config can safely be extended by any other config. (For example, in this repository, we also provide configs like "Next.js + Strictest", and so on.)

Is this accurate?


Option Violations

Based on this definition, it would seem that the following options are in violation:

    "esModuleInterop": true,
    "skipLibCheck": true,
  • 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. But skipLibCheck 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.

@orta
Copy link
Member

orta commented Feb 24, 2023

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.

skiplibcheck is a recommendation we make in tsc --init also, which is why it would be included in here too, I agree for power users that can occasionally suck but for most folk it means not worrying about how their node modules are set up too much

esmoduleinterop could maybe be removed in a year or two when node's ESM transition has been more settled but there's still a majority of commonjs out there

@Zamiell
Copy link
Author

Zamiell commented Mar 2, 2023

Thanks for the quick reply orta.

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

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:

  • When I upgrade the version of typescript in my project, I intentionally expect new type errors in my code - some of which will indicate real bugs in my codebase!
  • And subsequently, when I upgrade the version of @tsconfig/strictest in my project, I intentionally expect new type errors in my code - some of which will indicate real bugs in my codebase!

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!

@leite08
Copy link

leite08 commented Apr 14, 2023

+1 for the documentation part

@MicahZoltu
Copy link

I would like to echo the other commenters here, skipLibCheck seems to be out of place for the strictest tsconfig. IMO the point of this config should be to maximize the number of errors the compiler gives you when compiling your project. As a user, you can choose to fix those errors or override some of the compiler settings. I believe both skipLibCheck and esModuleInterop will strictly reduce the checking that the compiler does which is why they don't fit my vision of what this config is for.

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.

@orta
Copy link
Member

orta commented Apr 23, 2023

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 @tsconfig/recommended then make some docs changes to recommend ["@tsconfig/recommended", "@tsconfig/strictest"]

@MicahZoltu
Copy link

recommended + things which the TS team think are strictness flags but are too pedantic for most people to be in strict

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 pedantic and a new strictest could be created that lives up to its literal name and is the set of settings that will maximize the possible errors you can get?

@orta
Copy link
Member

orta commented Apr 25, 2023

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 tsc --init and strictest are the options the team felt was not worth giving to everyone by default.

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 strict: true.

Thus "strictest" is not recommended, but it is stricter - calling it @tsconfig/pedantic would make sense but I'm not breaking builds for that to rename (and there wouldn't be a "strictest" to replace it with because that is the same thing we have in strictest)

@MicahZoltu
Copy link

You make a good point about renaming being backward incompatible. How about a strictester config file? 😁

there wouldn't be a "strictest" to replace it with because that is the same thing we have in strictest

As per the discussion in this thread, a strictester would not include these settings, as they reduce the errors/warnings the compiler will give you (thus, it isn't the most strict config possible).

    "esModuleInterop": true,
    "skipLibCheck": true,

@orta
Copy link
Member

orta commented Apr 25, 2023

TBH - Those come from recommended, not from the pedantic flags, and would be removed if we split that out re: #156 (comment)

@MicahZoltu
Copy link

Ah, I see what you are saying. strictest is purely additive on recommended, and recommended currently has those two fields.

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 strictest minus esModuleInterop and skipLibCheck)? IIUC, if the user just did ["@tsconfig/strictest"] they would be missing all of the base stuff that recommended has and if they did ["@tsconfig/recommended", "@tsconfig/strictest"] they would end up with not the strictest set of settings.

@orta
Copy link
Member

orta commented Apr 26, 2023

Yep, I'd be saying they should have ["@tsconfig/recommended", "@tsconfig/strictest"]

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)

@MicahZoltu
Copy link

It sounds like the ship has sailed on renaming strictest, which is quite unfortunate because it is really frustrating having a thing called something it is not. I still would like to advocate for strictest actually being the strictest thing possible, but I don't have any more arguments other than "naming things accurately is important". If the maintainers of this repository are strongly against setting the following in strictest, then I suppose there is nothing more to debate 😢:

    "esModuleInterop": false,
    "skipLibCheck": false,

@Zamiell
Copy link
Author

Zamiell commented Apr 27, 2023

then make some docs changes to recommend ["@tsconfig/recommended", "@tsconfig/strictest"]

In the docs, we could reverse the order so that strictest comes first. That way, strictest could contain the strictest possible settings, and then recommended can contain values like esModuleInterop: true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants