denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.17k stars 5.16k forks source link

User libraries should be strict by default if written in TypeScript #3324

Closed Ciantic closed 3 years ago

Ciantic commented 4 years ago

My opinion is that:

Also I think there is no audience who loves working with non-strict TypeScript, it's mainly used by people who are migrating to TypeScript. Catering to "non-strict" TypeScript people is failure in waiting.

(Going in the reverse is more painful, because the libraries will then not type check, if we allow non-strict scripts to prevail.)

Point being that if you use TS you probably love the types and the strictness, if you don't you use JS anyway.

Opening this on suggestion by @kitsonk in #504

kitsonk commented 4 years ago

The TypeScript core team suggests that any new TypeScript project be enabled with strict that essentially it is a class of features that should have been in TypeScript from the start, but can't be, because of the large backwards compatibility situation. I believe DefinitelyTyped has gone to strict by default. When you scaffold a project using tsc the tsconfig.json defaults to strict.

Since TypeScript code in strict mode degrades well to sloppy mode, people can author for strict and still have it work in sloppy.

We support a custom tsconfig.json which would allow someone to run in sloppy mode, if they wish, as well as // @ts-ignore can be used to ignore specific errors. Everyone benefits from code being targeted at strict, so I am 👍 for enabling it.

rsp commented 4 years ago

I am strongly :+1: here, because I would love Deno to be the first runtime that is not suffering from The Billion Dollar Mistake, and without the strict flags we still get runtime errors like those, without any hint from the compiler that something can be wrong with the code:

Now is the only time when we can safely enable all strict flags by default. If we disable those flags now (which are not enabled by default by the TypeScript compiler itself only because of backwards compatibility but they are highly recommended by everyone) then after any significant amount of code for Deno is written and we need to maintain backwards compatibility, this decision will not be able to be changed later. Let's not make it one of the 10 things we regret about Deno few years from now.

Without flags like strictNullChecks we don't really have type safety at all. For example a function:

const f = (x: string) => x.toLowerCase(); // OK for compiler, unsafe on run time

doesn't guarantee that x is a string, but that it is either a string, a null or an undefined, which means that x.toLowerCase() can raise a runtime exception: TypeError: Cannot read property 'toLowerCase' of undefined - but to make it worse the compiler (or the editor) doesn't show any problem during the compilation which is very misleading.

If, on the other hand, TypeScript has all strict flags enabled, I still can have a function that takes null or undefined if I want but I have to be explicit about it, and I will get a compiler error when I try to invoke toLowerCase() method on something that is not guaranteed to have it:

const f = (x: string | null) => x.toLowerCase(); // COMPILE TIME ERROR
const f = (x: string | null) => x && x.toLowerCase(); // OK for compiler, safe on runtime

(instead of string|null or string|undefined people often use Maybe<string> or M<string> so it's not much more typing)

I believe it is a false sense of safety to use TypeScript without strict null checks, I even gave a talk titled: Call me irresponsible if I ever crash on null or undefined in JavaScript or TypeScript exactly about this very topic.

See also my comment in issue #504 here for some more details.

I am 👍 for all strict flags enabled in Deno for all TypeScript code.

brandonkal commented 4 years ago

FWIW I'm not a fan of forcing strict mode. It often means spending more time writing types with little gain. The compiler is rather good at inferring. StrictNullChecks should be on though.

Also if a library author doesn't use strict mode but you do, it won't work out.

ry commented 4 years ago

Thanks to @maxmellen for this work.

Let's see how annoying this is in practice... I am open to making strict mode non-default again if it turns out to be too verbose.

Ciantic commented 4 years ago

I would expect it to be annoying when migrating, because strict mode will find a lot of subtle bugs (and any assumptions that shouldn't be there).

With all new projects it should be easy since strict: true is the default (tsc --init) with TypeScript projects anyway.

gewoonwoutje commented 4 years ago

FWIW I was a bit surprised seeing compiler errors pointing to external URLs after updating from v0.33 to v0.34. image

This could have a huge impact on type safety in third party libraries, I definitely see value in that. I do think that a setting to disable the strict check for (specific?) external libraries would be nice to have.

brandonkal commented 4 years ago

That's just asking for trouble. If one of your deps is not strict, your whole project cannot be.

kitsonk commented 4 years ago

Per-module permissions/config/etc. are a rabbit hole I don't think we really want to go down. Non-strict code does no one any favours. If people can't get the upstream code to be strict, using tsconfig.json and setting "strict": false is what people should do in my opinion.

Ciantic commented 4 years ago

Whole purpose of this issues is to force TypeScript to be strict, it's the default. It will be painful with user code that is non-strict.

Problem is that there is now bunch of shoddily built TS deno code out there, it will be painful at the beginning, but strictness is default for TypeScript.

gewoonwoutje commented 4 years ago

True, and this should (hopefully) be a temporary problem anyway.

It is still a bit weird to explicitly turn of strictness in my entire project, because a library (I trust) has forgotten a type annotation for a function that may not even be exported to the module.

Skillz4Killz commented 4 years ago

+1 for strictness. If you use a non-strict lib, perhaps consider making a PR to improve that lib to make it strict.

maxmellen commented 4 years ago

I think it's worth mentioning that DefinitelyTyped's stance on this issue is that they consider that all the packages hosted in their repo should have noImplicitAny, noImplicitThis, and strictNullChecks enabled. (source)

Here's the default tsconfig.json file you get when adding a new package into their repository using the dts-gen utility:

{
    "compilerOptions": {
        "module": "commonjs",
        "lib": [
            "es6"
        ],
        "noImplicitAny": true,
        "noImplicitThis": true,
        "strictFunctionTypes": true,
        "strictNullChecks": true,
        "baseUrl": "../",
        "typeRoots": [
            "../"
        ],
        "types": [],
        "noEmit": true,
        "forceConsistentCasingInFileNames": true
    },
    "files": [
        "index.d.ts",
        "mypackage-tests.ts"
    ]
}
brandonkal commented 4 years ago

That doesn't really translate though as DefinitelyTyped is types for exported public functions. And it is for code that is not strict (JavaScript). Those rules don't mean all normal TypeScript code should be noImplicitAny especially for internal implementations.

maxmellen commented 4 years ago

@brandonkal You have a point there, although my comment was more of a reaction to @gewoonwoutje's comment.

What I wanted to stress is that it seems the TypeScript ecosystem is moving in the direction of users being able to expect that libraries expose strictly typed interfaces which can be used in strict as well as non-strict codebases rather than users having to deal with libraries possibly having non-strict types and figuring out how to use such libraries in strict codebases.

I'm also not aware of any TypeScript or Deno feature that lets a user specify which files should be considered part of a codebase's public API and which should be considered internal to that codebase, especially considering Deno does not require the use of any project configuration file.

I would argue that defaulting to strict makes new code more interoperable with the rest of the ecosystem by default.

keroxp commented 4 years ago

Recently I found many third-party library massively broken by that changes. I think --strict is too opinionated. It may be acceptable for stand-alone project but not for runtime, certainly.

The best advantage of TypeScript is that users can select suitable typing power for their project. Indeed we provide custom tsconfig.json for modules. But there are no way that "strict" module and "non-strict" module coexist. "strict" code can't be loaded by "non-strict" deno execution.

I'm OK to enable strict mode for deno's internal code (cli/std) as it is the stand-alone project and should be fully-maintained.

Again, "strict" is too opinionated for general purpose TypeScript runtime. Deno shouldn't force user to use specified compiler options.

IMO: At least --no-implicit-any should be disabled by default. It broke too many existing code bases.

Ciantic commented 4 years ago

This is exactly what this change is supposed to prevent, proliferation of non-strict TypeScript. It was expected that this change breaks everything, because that had already happened.

Non-strict typescript is an anomaly, not many people write it, but deno used to force it down as a default. Whereas the strict TypeScript is default with all new projects created with tsc.

Deno shouldn't force user to use specified compiler options.

Notice! Non-strict actually requires more flags! And we can't agree what flags are "the correct flags". I think the tsc default (strict: true) is sanest way.

kitsonk commented 4 years ago

Deno shouldn't force user to use specified compiler options.

It always has, and always will. You can use a tsconfig.json if you don't agree with those, up to the point where the options impact Deno's ability to consume the emitted JavaScript.

brandonkal commented 4 years ago

I agree that noImpicitAny should be disabled by default. It breaks things without any real gain. People start typing any to shut up the compiler so it doesn't help make libraries better or more type safe. The other strict options are fine.

kitsonk commented 4 years ago

People start typing any to shut up the compiler so it doesn't help make libraries better or more type safe.

Except making someone be explicit that they are writing un-types safe code. It is like unsafe in Rust. Implied unsafe-ness is a lot worse than explicit unsafeness. I would like people to make the conscious decision to type any to shut the compiler up instead of just implying it.

Also, we shouldn't get into cherry-picking things like this. strict is the TypeScript's version of "if we wouldn't break backwards compatibility we would require this". Implicit any would be one of the things they would do away with if they could, otherwise why use a type system?

brandonkal commented 4 years ago

otherwise why use a type system?

Those are the reasons I use TypeScript. Outside of that there is no bug that TypeScript would catch that could not be found faster with a linter + unit tests.

I like when I can rename a JavaScript file to .tsx and have it just work. I believe the original idea was that any valid JavaScript is valid TypeScript. noImplicitAny breaks that idea completely and it is just one of the reasons it is a bad option.

kitsonk commented 4 years ago

Those are the reasons I use TypeScript. Outside of that there is no bug that TypeScript would catch that could not be found faster with a linter + unit tests.

You can get all of that in your editor (VSCode) without using TypeScript, just in JavaScript and actually have nothing to do with running TypeScript, they all have to do with the development workflow. You can run JavaScript under Deno. You can turn strict mode off in Deno.

Because not using strict is potentially a common use case. Providing a top level flag, instead of having to learn how to load a tsconfig.json might be the right solution. --no-strict or --allow-sloppy could be warranted. I personally prefer --allow-sloppy.

I believe the original idea was that any valid JavaScript is valid TypeScript.

I don't want to get into debating the design goals of TypeScript here, but the goal, from the conversations I have had with the core team, is that TypeScript is a superset of JavaScript syntactically, and that the type system should be fully erasable from an emit. That continues to hold true. There is valid JavaScript that breaks under TypeScript all the time, like trying to access properties that don't exist, or non-logical comparisons. So that noImplicitAny "breaks" something that was never intended 🤷‍♂ .

brandonkal commented 4 years ago

VSCode provides IDE features using the the TypeScript server. So the assertion that you can get all these features without TypeScript is a false one. I'm well aware you can use it indirectly via checkJS (via a less-integrated syntax) and DT.

Of course there are other cases where good JavaScript breaks as TypeScript. But noImplicitAny is the main one that makes TypeScript conversion non-trivial. Accessing properties that don't exist is not valid JavaScript (meaning a sane code review wouldn't allow it, of course the syntax is valid).

I didn't comment to convince someone in the strict always camp to change their mind. My point is that for many users noImplicitAny is too opinionated and enabling it by default should be reconsidered considering it provides near-zero improvement. It is one thing to have it enabled for tsc, where it is a glorified linter. It is a whole other thing to have it enabled where it breaks runtime behaviour.

Consider a user who uses a working library. Now they have to fork the library, litter it with : any and keep that long running fork. You can't expect library consumers to create types for internal implementations of third party code. Developer efficiency goes downhill. So now you have a bunch of any and no idea if it is legitimate any or a forced any. I don't see how that story is a good one.

Ciantic commented 4 years ago

Consider a user who uses a working library. Now they have to fork the library, litter it with : any and keep that long running fork. You can't expect library consumers to create types for internal implementations of third party code. Developer efficiency goes downhill. So now you have a bunch of any and no idea if it is legitimate any or a forced any. I don't see how that story is a good one.

It was already pointed out that biggest collection of library definitions (DefinitelyTyped) has noImplicitAny, so from library authors point of view that is already a requirement.

ry commented 4 years ago

Being overly strict is painful for a scripting language. I would be using a scripting language to get things done quickly. If I wanted to be super-specific about what I was doing, I could just write it in Rust.

This is unrelated to strict mode - but just a general comment about overly pedantic rules: in the Deno code base we have a lint rule which requires writing the return types. So for many async functions we end up writing async function foo(...): Promise<void> ... The Promise<void> is just boilerplate - it make code unenjoyable to write.

Deno should be like a toy. Easy and simple to use.

I acknowledge that there are going to be situations where people want to change tsconfig - but we should strive to make that the minority case. Most people should be able to start script with one file, most projects should not have to ever think about configuration.

kitsonk commented 4 years ago

@ry if we reverse this, and I can understand the motivation, using strict should be fictionless as well... an easy step to upgrade your experience... If we introduced a --strict top level flag, so people can use it without having to load a config file, that would be a win.

Also, I would recommend we would continue to run --strict within the Deno code, including std.

Skillz4Killz commented 4 years ago

This is unrelated to strict mode - but just a general comment about overly pedantic rules: in the Deno code base we have a lint rule which requires writing the return types. So for many async functions we end up writing async function foo(...): Promise<void> ... The Promise<void> is just boilerplate - it make code unenjoyable to write.

@ry Why not disable that lint rule and allow TS to infer those? TS inference is really good, I have never ever needed to provide the return type of a function like that and I use TS strict mode always. It seems like that lint rule is causing more harm than helping.

Deno should be like a toy. Easy and simple to use.

100% agree. But every toy has certain standards, for example choking hazards for kids.

If I go to a toy store, I expect the toys there to meet a certain standard. It would be nice to have the ability to say that all Deno libs meet the TS strict standard.

ry commented 4 years ago

The fact that TypeScript is defaulting to strict mode, is a strong suggestion that we should as well. The incompatibilities that @keroxp is experiencing will go away as people update code to work with latest versions of deno - it can be chalked up to unfortunate growing pains.

We'll leave strict mode on by default for now - in particular @piscisaureus and @bartlomieju both are in favor of it. (And certainly we will always maintain very strict linting rules for Deno's internal code)

I worry about the user who has prototyped something quickly in JS, and now wants to start slowly adding types - but finds that when they rename their .js to .ts they suddenly have to update every function definition. It seems like a major benefit of typescript is lost if it's "all or nothing".

brandonkal commented 4 years ago

As I mentioned above, strict - noImplicitAny would be the best default especially for prototyping (you can let TypeScript infer types in most cases). The latter can be found via an eslint rule or a flag to disallow implicit any.

nayeemrmn commented 4 years ago

I worry about the user who has prototyped something quickly in JS, and now wants to start slowly adding types

No need to worry, they have config. The temporary transition phase should of course be the one that requires config. That way the config is temporary.

brandonkal commented 4 years ago

Do you honestly expect someone to write a config file to prototype something? That's an unfair justification @nayeemrmn and just a bad user experience. Config is rarely ever temporary.

nayeemrmn commented 4 years ago

Do you honestly expect someone to write a config file to prototype something? That's an unfair justification @nayeemrmn and just a bad user experience. Config is rarely ever temporary.

@brandonkal Umm, no... the prototyper is using JS. We're talking about the person progressing from that, aren't we? :) Also I'm in favour of a flag.

Config is rarely ever temporary.

What does that mean here? I'm saying that strict is the one you're hardly going to go back from, non-strict might be temporary so it should be the specified one.

hayd commented 4 years ago

Config is rarely ever temporary.

This was @kitsonk 's suggestion of --allow-sloppy (alternative to a config), and I suspect calling it that will mean people will want its use to be temporary!

Potentially there could be a message on strictness errors suggesting this flag.

Ciantic commented 4 years ago

I agree with @nayeemrmn logic here.

When you are gradually adding types (converting between JS and TS) you can just add tsconfig.json with non-strict flags, that is already what TypeScript suggest you to do.

When you are done you can just remove tsconfig.json because conversion is done, and everything is strict!

keroxp commented 4 years ago

I agree with @ry and @brandonkal. And the real problem is that there are no way to apply configuration for each remote modules that depend on. There are incompatibility between strict code and "sloppy" code (for each other. Some valid code in strict may be invalid in non-strict mode). Custom tsconfig.json doesn't provide custom strictness to users. This means there are no choice to write "sloppy" code eventually.

IMO: If it were really the ideal and correct way of programming, its name won't be "strict". IMO2: Disabling --no-implicitAny is better landing point of this issue currently. I'd been really tired for writing any or void on argument and return type.

Ciantic commented 4 years ago

Some valid code in strict may be invalid in non-strict mode

Really? What is that code you write in strict mode which is not usable in non-strict mode?

However the reverse is true: non-strict code is not usable in strict mode. This is the reason DT defaults to having more flags turned on.

If the deno defaults to non-strict mode the libraries start to gather non-strict code and you can never use those libraries with strict mode.

keroxp commented 4 years ago

@Ciantic

https://github.com/keroxp/deno-redis/issues/50

Summary is here:

export type BulkResult = string | undefined;
export type RedisRawReply =
  | ["status", string]
  | ["integer", number]
  | ["bulk", BulkResult]
  | ["array", any[]]
  | ["error", Error];

function rawReply(): RedisRawReply {
  return ["bulk", "result"]
}

function getBulkResult(val: string | number | undefined): BulkResult {
  const [_, reply] = rawReply();
  if (typeof reply !== "string" && reply != null) {
    throw new Error();
  }
  return reply;
}

With strict:false tsconfig.json

deno run -c tsconfig.json strict.ts
Compile file:///Users/keroxp/src/deno-pg/strict.ts
error TS2322: Type 'string | number | any[] | Error' is not assignable to type 'string'.
  Type 'number' is not assignable to type 'string'.

► file:///Users/keroxp/src/deno-pg/strict.ts:18:3

18   return reply;
     ~~~~~~~~~~~~~

I don't what happened clearly. It seems that if statement was wrong but no error shown in strict mode.

Ciantic commented 4 years ago

@keroxp that has nothing to do with noImplicitAny, I pasted your code here:

https://www.typescriptlang.org/play/#code/KYDwDg9gTgLgBDAnmYcBCBXANgawErADO28AvHITFAJYB2A5nAD5wa0AmwAZncOwNwAoUJFgJkqAu2qE8AQwDuBMFkRxSguMzgBtAESU5MDIT0AaClTr0Aupu366MYPWBRzcWhgC2AIzd2Wiz6vtg4Hpi4BMRYMIEOenJQUHKIHnK0iDo28cF6blDQHgCiydA2QoJcbADGMNQQtHApSsAqiAAUAJQAXHBSMvKt7XAA3vZQwMZQTSFhHnqTMTB6dgC+glW19Y1wrjCR+EQkHQBuclh9lDQM2l5+btpsnDy0fL3oYdEkY-Y1jZRdAB9CyTdo2dTNRTKVTdIRaahcOAdJAoCBIsGqOAAQlI5AMVgYejgADISc02ljcZ5sFgur8tFoYAALQoKTzAdmlQpQOH2DZaSbTJqYxBCNZAA

And tune "noImplicitAny" on or off, it won't give errors in either mode. No errors in strict or otherwise.

None in this thread is actually anymore advocating for completly non-strict code btw, we are talking about noImplicitAny here mostly.

kitsonk commented 4 years ago

IMO: If it were really the ideal and correct way of programming, its name won't be "strict".

It is, according to the TypeScript core team, it is the correct way of programming. I've talked to them personally. Just like "use strict"; pragma in ECMAScript doesn't mean that strict mode isn't the ideal way of programming JavaScript, it was implemented to keep backwards compatibility and correct things that were actually regrets.

It is true that some strict code will break in sloppy mode. The specific example given undefined and null are not distinct types, so the comparison (reply != null) does not narrow the type to "everything not undefined). Because sloppy mode doesn't understand it, it re-widens based on the comparison outside the block, leaving you with everything possible. You can see when the assignment occurs, even though you specified undefined, that the return type of the function is string. It is exactly these type of things that were corrected in strict mode.

vwkd commented 3 years ago

I don't get why people allow TypeScript to prevent them from running code. You're not forced to listen to the TypeScript errors. You can always run your code using Deno's --no-check flag (which IMO should have been the default, but that's another topic).

TypeScript should be seen like a linter. They're helpful hints during development, but they don't prevent you from running the code at all at any time.

This is also why I'd vote for strict type checking, including no-implicit-any active by default. This doesn't "break" your project, it just prints squiggly lines that you can ignore if you want. Or you could actually use the squiggly lines to find the things that you forgot to type, which you should do, because this is the whole point of using TypeScript.

kitsonk commented 3 years ago

We have been run with strict on by default for an extended period of time and we have forgotten to close this issue.