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

Skip type checking by default #11340

Closed
kitsonk opened this issue Jul 9, 2021 Discussed in #8549 · 61 comments
Closed

Skip type checking by default #11340

kitsonk opened this issue Jul 9, 2021 Discussed in #8549 · 61 comments
Assignees
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Milestone

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Jul 9, 2021

Discussed in #8549

Originally posted by kitsonk November 30, 2020
Instead of --no-check consider type checking TypeScript as an opt-in with a --check option.

The core team has discussed this and reviewed the feedback from the community. Based on that, we are going to move forward with the following:

  • The --check flag will be implemented for the appropriate subcommands. This will use the built-in tsc for type checking and use tsc for the emit (except for bundles where we will use swc). We may in the future use swc for the emit here as well. This is basically the current default behaviour of Deno.
  • The --no-check flag will be deprecated and issue a warning that it is deprecated. The behaviour will not change from its current for now.
  • The default behaviour of the command line will be changed so that the code is not type checked and emitted using swc (this is basically the behaviour of --no-check now).

We will raise the PRs necessary to implement this and tentatively aim to introduce this in 1.13.

Why are you doing this?

The goal of the Deno CLI is to be a fun and productive scripting language. A user should be able to hack on some code and quickly see the results of their endeavour. The more "hurdles" that are in the way, the less fun and productive the experience becomes. We ❤️ TypeScript, we want people to use TypeScript more and more, but there are two major challenges with that:

  • Type checking is relative slow and expensive. We have tried (almost) everything to speed it up (see: TypeScript compiler in Rust #5432) but a relative small program still takes a second or two to type check, while just transpiling the TypeScript with --no-check takes under 100ms.
  • Deno terminates if it encounters a type check diagnostic. That is on us. We should have maybe always treated them as warnings and still executed the output anyways, but we didn't. We could of course change that now, but the problem then is the point above. We don't feel there is a huge amount of value to users to just address this point.

While on the surface it might seem like we are "watering" down Deno, we are motivated to make it easier to use Deno. If you are just starting out, maybe not having used TypeScript in strict mode before, or are just trying to hack a few lines of TypeScript and don't have all of your type annotations right, or you have imported a 3rd party library whose globals conflict with another 3rd party library, you have to spend all your time sorting that out to just find out if your code runs. Again, that isn't fun for anyone and certainly isn't productive.

Also, we have tried to put a lot of effort into the language server, and realistically that is where most people type check their code anyways, in their editor, which means to the majority of people type checking is not the default need they have when running on the command line. It is something they need as a final check, maybe in their CI, maybe before they commit, not every time they want to see if the code runs.

So, we acknowledge that in the discussion there wasn't overwhelming broad support for this, but we think we need to do this not for those already using Deno on a daily basis, but for those experimenting with Deno and TypeScript.

@kitsonk kitsonk added feat new feature (which has been agreed to/accepted) cli related to cli/ dir labels Jul 9, 2021
@kitsonk kitsonk added this to the 1.12.0 milestone Jul 9, 2021
@kitsonk kitsonk self-assigned this Jul 9, 2021
@caspervonb
Copy link
Contributor

caspervonb commented Jul 9, 2021

It's a big change and seems rushed but I think I'm fine with it as long as it's in a major release.
Altho the reasoning that people just use lsp anyway is odd but as long as the type-checker is still there it isn't a big deal.

We may do all of this for 1.12, or we may only do the first two points and delay the third for 1.13.

This seems overly optimistic, this is such a big change in how the CLI works that it really should be major release (2.0) imo.

If you are just starting out, maybe not having used TypeScript in strict mode before

Is --strict still going to be the default?

And just want to stress again, I'm not that agains the change itself but we cannot under any circumstance change such a fundamental thing in a minor release.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

It's a big change and seems rushed but I think I'm fine with it.

It has been discussed since Nov 2020. That does not fit my definition of rushed.

This seems overly optimistic, this is such a big change in how the CLI works that it really should be major release (2.0) imo.

While it is a significant change, it is one in the favour of the user. If it was a change the stuff that used to work broke, then I would be more concerned. The worst case is that what might not have run because of diagnostics will run and throw a runtime error. It does not impact what code is runnable under Deno, and it only requires a --check flag return to the way they were. It is my opinion (and others) that that doesn't warrant a "major" release. We have changed the way we have run tests extensively, which arguably has a greater impact on existing code without a major release.

Is --strict still going to be the default?

Yes. This does not change the ideas behind #7732 which would be to have a single way to "type check" code in Deno.

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

I don't see why we are changing Deno's defaults to target new users that may not use the runtime for more than a few minutes, Deno's defaults should be targeted at the people who use it daily and on a regular basis, not people just trying it out. Also, the main idea of Typescript is to have types, disabling type checking by defaults basically leaves useless type annotations that might aswell be JSDoc comments.

you are just starting out, maybe not having used TypeScript in strict mode before, or are just trying to hack a few lines of TypeScript and don't have all of your type annotations right

This can easily be solved by running with --no-check or using @ts-ignore comments, which we're made for this purpose.

or you have imported a 3rd party library whose globals conflict with another 3rd party library

Frankly I don't see this being a reality, most popular Deno libraries are well put together and I don't think its likely for new users to find bad libraries their first time using Deno. And I'm yet to see someone actually encounter such an issue.

Also, we have tried to put a lot of effort into the language server, and realistically that is where most people type check their code anyways, in their editor

Not sure why there's still deno fmt aswell, since most editors have built in formatters. And regardless, editors and the LSP are not perfect. Although rarely, I've had Deno code that VSCode(+ the Deno extension) brought up no problems or warnings with, but still failed due to types or some other issue when ran.

This seems like a somewhat hasty decision without much solid reasoning and not much support from the community. And by hasty I mean that it was discussed for what amounts to few days back in November and December 2020 and there was no further discussion.

@caspervonb
Copy link
Contributor

caspervonb commented Jul 9, 2021

It has been discussed since Nov 2020.

Yes and I thought we had moved on from it back in November.
But this has apparently been decided today and is tagged for the release which is in ~3 days so it is rushed.

The worst case is that what might not have run because of diagnostics will run

Shouldn't be treated any different than a programmatic API.
Changing the default is a breaking change.

For example, this command will break completely:

deno test --no-run

We have changed the way we have run tests extensively, which arguably has a greater impact on existing code without a major release.

We've kept the old behaviour of all the stable flags for backwards compatibility in all the subcommands, even when it doesn't make sense.

@somebody1234
Copy link

tl;dr: defaults should be the most common usecase, not a tailored specifically to increase user adoption (and only to increase user adoption)
tl;dr 2: i don't believe a user that regularly "hacks" on code means they're so insanely impatient that a 10 second startup time that occurs once is the end ofd the world

ok so... you could just suggest using --no-check when something fails to run.

realistically 99.9999% of users will not be hacking on multi-million line projects, and therefore typecheck time should be within, say, 10 seconds. it's far from instant but i believe the drastic increase in the probablity of things actually working will be worth the tradeoff for most people. i highly, highly doubt it would make much of a dent in user retention or whatever - this is not a website, so i believe there is no real decrease in ux with a one or two second delay.

it's important to note:

  • if they don't want typescript, they can simply just not write it in typescript in the first place, no?
  • if they want a shorter dev cycle, is this not what --watch is for?

@borekb
Copy link

borekb commented Jul 9, 2021

This is a good decision, IMO. 👍

In Node.js world, I also don't want a full typecheck when running TypeScript code – that is reserved for an IDE or an explicit yarn tsc command or CI, as you said. When I run a .ts / .tsx file, I just want the types stripped via Babel or esbuild or ts-node --transpile-only or similar, which is a trivial operation that takes milliseconds. It's also what frameworks like Next.js or Gatsby or CRA all do now.

To be clear, we're quite strict when it comes to our codebase – TS is in strict mode, we have additional ESLint rules and other static checks but to run the code, we just want the types stripped. TypeScript is mostly a "linter" for us.

Glad to see Deno thinking about it the same way, we already had --no-check everywhere.

@capaj
Copy link

capaj commented Jul 9, 2021

Yes, this is the sanest default. Typesafe code is a nice aspiration, not a must-have.
If someone wants their code 100% typesafe they can use typescript to check it. Deno is a JS runtime. It can never be a TS runtime as TS does not exist at runtime.

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

Yes, this is the sanest default. Typesafe code is a nice aspiration, not a must-have.
If someone wants their code 100% typesafe they can use typescript to check it. Deno is a JS runtime. It can never be a TS runtime as TS does not exist at runtime.

What? This issue disables typechecking for typescript by default, javascript does not have that ability.

@littledivy
Copy link
Member

I'm in favor of this decision 👍

Deno subcommands have been very explicit with what they do. Eg: deno fmt won't ignore node_modules/ by default unless you want it too (via --ignore)

Similarly with this, deno run will simply run my code and when I want to type check it (in the CI or locally or whatever) - I can just pass --check! That's how I (personally) want it to work.

@somebody1234
Copy link

@littledivy okay, sure, but what if i don't want the code to work if it isn't supposed to work in the first place?

@somebody1234
Copy link

@borekb

trivial operation that takes milliseconds

you're over-exaggerating.
emit may be around, say, 10 times faster than typecheck, but it's still nowhere near that fast for anything larger than, say, a thousand lines.

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

I'm in favor of this decision

Deno subcommands have been very explicit with what they do. Eg: deno fmt won't ignore node_modules/ by default unless you want it too (via --ignore)

Similarly with this, deno run will simply run my code and when I want to type check it (in the CI or locally or whatever) - I can just pass --check! That's how I (personally) want it to work.

I disagree. I would expect deno run to type check my typescript code. It not typechecking it by default would just confuse me if i was a new user, and this change seems to be targeted at new users. Why would I use typescript if I didn't want my code to be typechecked? Makes no sense, if I didn't want type checking i'd just use javascript

@somebody1234
Copy link

ok so my viewpoint is, typechecking is slow, but that's about the only negative. typechecking is desirable behavior. there is almost no situation in which you'd want a program that semantically does not work to be running, and very few situations in which the 10 second, or one minute compile time is the difference between success and failure.

basically:

  • the only advantage here is faster time to program execution
  • the errors mentioned in the original post generally mean the program will Not Work™ in the first place
    • and even if you wanted it to work, that is the exact point of // @ts-ignore, no?

@borekb
Copy link

borekb commented Jul 9, 2021

@somebody1234 I don't know how Deno does it but even large Next.js projects transpile "immediately" because they do it per file – if I change some-file.ts in a folder where I have hundreds of other .ts files, it really is a couple of milliseconds to transpile that single file. Doing full tsc emit would indeed be slow, which is why they just don't do it.

(Per-file transpilation takes a few TS features away, most notably const enum but it's totally worth it in practice in my experience.)

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

@somebody1234 I don't know how Deno does it but even large Next.js projects transpile "immediately" because they do it per file – if I change some-file.ts in a folder where I have hundreds of other .ts files, it really is a couple of milliseconds to transpile that single file. Doing full tsc emit would indeed be slow, which is why they just don't do it.

(Per-file transpilation takes a few TS features away, most notably const enum but it's totally worth it in practice in my experience.)

Next.js and Deno are massively different workloads, you're practically comparing frontend to backend, doesn't make much sense as a comparison and you have to keep in mind that Next.js also has the React build step to optimize for which would make skipping type checking much more reasonable, this build step more often than not does not exist in Deno. And Deno uses tsc for typechecking and swc for emit, this is faster than using just tsc for both type checking and emit, which is what Next.js would do if it did typechecking.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

it isn't supposed to work in the first place?

When you start importing modules not written for Deno but run perfectly fine on Deno and the compliet start complaining about NodeJS.Process not being defined or being duplicatly defined, you realise that there are limits that aren't easily overcome. "Isn't supposed to work" is a very relatively concept.

Again to be clear, if someone wants the existing behaviour, all the need to do is type --check. We might also consider adding deno check or renaming deno cache to deno check and enabling checking by default on that.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

And Deno uses tsc for typechecking and swc for emit

We currently use the tsc emit when we type check, except when we bundle. We only use swc for individual modules when --no-check is used.

@somebody1234
Copy link

but run perfectly fine on Deno

@kitsonk i don't know about you, but imo if the type definitions are definitely conflicting with something, i'd trust the compiler that has read the entire source of the library over someone just going "nah, trust me, it'l all be fiiiiiine"

basically, if it "works perfectly fine", it shouldn't give type errors in the first place

@sebastienfilion
Copy link
Contributor

The only opinion I have is that we need this change to extremely obvious to every users. Maybe a warning each time you run a TS file without the —check flag or something like that.
I don’t think all users either read the manual properly or knows what they’re doing enough to realize what’s going on.
All in all, I would prefer that type errors are shown as warning and only fail if we pass a flag. Something like that.

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

All in all, I would prefer that type errors are shown as warning and only fail if we pass a flag. Something like that.

Type errors as warnings seems like a good compromise if core team isn't willing to not go forwards with this.

@nayeemrmn
Copy link
Collaborator

All in all, I would prefer that type errors are shown as warning and only fail if we pass a flag. Something like that.

I think this is the worst thing we can do. Seeing a wall of errors and then the output of your code is jarring for everyone, both the person who "just wants their code executed" and the person (more typical of those using a typed language in the first place) who wants their expectations to be met before anything is ever run. You either care about type errors at the moment or not.

@sebastienfilion
Copy link
Contributor

I think this is the worst thing we can do. Seeing a wall of errors and then the output of your code is jarring for everyone

Fair, but we could address my concerns instead of dismissing a fleeting thought.
This change has to be absurdly obvious.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

but run perfectly fine on Deno

@kitsonk i don't know about you, but imo if the type definitions are definitely conflicting with something, i'd trust the compiler that has read the entire source of the library over someone just going "nah, trust me, it'l all be fiiiiiine"

basically, if it "works perfectly fine", it shouldn't give type errors in the first place

I don't want to sound condescending, but if you haven't encountered type errors that have nothing to do with the type safety of your code, you haven't done enough Typescript, or enough Typescript with Deno. Many codebases are littered with // @ts-ignore or // @ts-expect-error or the not-null assertion operator. And if you have run into a good 'ole @types conflict that is astounding. Try doing some isomorphic code between Deno, Node.js and a browser. It can easily turn into a nightmare. I have been going TypeScript since around 2011. I love it, but even these days there are things that get in the way.

We are not suggesting taking away type checking, we are suggesting it is not the default behaviour. And I won't name names, but it was a core Typescript team member who question why Deno type checked by default back almost a year ago that led to opening the discussion in the first place.

@grian32
Copy link
Contributor

grian32 commented Jul 9, 2021

but run perfectly fine on Deno

@kitsonk i don't know about you, but imo if the type definitions are definitely conflicting with something, i'd trust the compiler that has read the entire source of the library over someone just going "nah, trust me, it'l all be fiiiiiine"
basically, if it "works perfectly fine", it shouldn't give type errors in the first place

I don't want to sound condescending, but if you haven't encountered type errors that have nothing to do with the type safety of your code, you haven't done enough Typescript, or enough Typescript with Deno. Many codebases are littered with // @ts-ignore or // @ts-expect-error or the not-null assertion operator. And if you have run into a good 'ole @types conflict that is astounding. Try doing some isomorphic code between Deno, Node.js and a browser. It can easily turn into a nightmare. I have been going TypeScript since around 2011. I love it, but even these days there are things that get in the way.

We are not suggesting taking away type checking, we are suggesting it is not the default behaviour. And I won't name names, but it was a core Typescript team member who question why Deno type checked by default back almost a year ago that led to opening the discussion in the first place.

Yes and that's fine, thats the purpose of // @ts-ignore and others, that's why they exist, don't change Deno's defaults to solve an issue that has already been solved.

@sebastienfilion
Copy link
Contributor

Seeing a wall of errors and then the output of your code is jarring for everyone, both the person who "just wants their code executed" and the person (more typical of those using a typed language in the first place) who wants their expectations to be met before anything is ever run. You either care about type errors at the moment or not.

I really don't understand why the solution for that problem isn't just to use JavaScript. If you're not gonna care about types, change the t for a j and voila.

@somebody1234
Copy link

somebody1234 commented Jul 11, 2021

@ebebbington please also note that not everyone here is on the discord, and not everyone is awake at the same time, making discord the opposite of the ideal place to discuss. note also that there's a feature on github literally called discusssions, which would probably be a better place for this.

edit: it seems that there is indeed a related discussion (#8549 ), we should probably just move all conversation there

@somebody1234
Copy link

@peerreynders again, the issue is not about dynamic vs static typing. if you want dynamic typing, as said countless times above, just use javascript.

re: not wanting to type check on every single pass, fun fact, you can pass --no-check to deno to skip that. the point is that that should be the exception, not the default.

@somebody1234
Copy link

@caspervonb my point still stands, all valid js code is valid ts code. this does not mean that there will be no ambiguities (like that case) - generic syntax in particular is infamous for being ambiguous

@somebody1234
Copy link

@vwkd note that vim does have support for LSP, so deno LSP should work with relatively minimal changes (if any).

note also that deno has its own language server which is considerably different from typescript's default, so "just using an IDE with typechecking" and "using the deno language server" will be considerably different in terms of development

for example, one key point to note is that deno does not use DefinitelyTyped, and uses URL imports rather than node modules for dependencies (meaning any built-in typescript extension would almost certainly not work)

@somebody1234
Copy link

Were my code JavaScript, my IDE wouldn't give type information because JavaScript assumes there isn't any

@DefinitelyMaybe this one is just objectively false because vscode typechecks (and shows errors) for javascript by default, since its typescript extension is built in

@somebody1234
Copy link

somebody1234 commented Jul 12, 2021

crosspost from the discussion. i'm fine with deleting this if activity at the discussion picks up again.

since some people don't want to discuss in the issue, here's a summary of my viewpoint:

  • typescript is designed to be type checked. intentionally skipping typechecking just completely defeats the purpose of using typescript in the first place
  • if you have type errors and think your code works, that's what // @ts-ignore is for.
    • (since i mention this here, i will not be listing this point below.)

on common arguments for no typechecking being the default behavior:

  • "my ide already typechecks for me"
    • good for you, of course this is the case for everyone else wanting to work on every other deno project too. of course
  • "typechecking is slower than not typechecking"
    • yes, but does the minute or so saved every time you deploy make it worth losing the correctness guarantees?
      • not to mention it shouldn't be too hard to deploy with almost zero downtime
    • if this is a local project, i believe that's what filesystem watchers are for
  • "you should be using the language server for typechecking"
    • note that deno is a server software. this generally means it will run on a server, which is generally headless
      • as such, while you can install vscode on the server, i'm not quite sure that's a good idea
      • while you shouldn't need to edit code on a server, please do note that that's not a reason to just
    • also note that language servers are only supported by default by vscode. other IDEs may have support, but considering LSP is still new enough for implementations to not be mature, you're effectively heavily restricting the choice of IDE.
      • note also that some may consciously choose not to use a LSP extension, or the deno LSP, because of performance/memory issues
  • "i don't want to convert a js project to ts"
    • you're not supposed to. just use // @ts-ignore on any type errors you encounter, and any on any variables you can't be bothered to type properly
      • more importantly, it's not supposed to be hard to add basic types anyway
  • "type annotations don't guarantee correct code"
    • they eliminate a very large proportion of bugs, and that's the important thing.
    • this is like saying "eating healthy won't prevent illness, so i'll just eat junk food all day every day"
  • "type checking only helps while writing code"
    • it's not supposed to help when building code. it's supposed to stop clearly incorrect code from running.
      • as mentioned above, if you're absolutely, completely sure that the code you wrote is correct, // @ts-ignore will do the trick
  • "linters and formatters both are invoked by the user so typechecking should be too"
    • linters and formatters have exactly 0 to do with the correctness of the code. linters suggest recommendations to make the code more maintainable; formatters make the code more readable and the formatting consistent.
  • "in other words for beginners focusing on types (and specifically type errors) is distracting/confusing"
    • the solution is simple; just don't use typescript. it's completely contradictory using a typechecked language if you think it's confusing
      • note also that i believe that it's confusing/distracting because they're just taught types wrong.
  • "type checking may fail even though the code is correct. a single wrong type annotation anywhere in the code will make it fail"
    • yes, that is because your code is wrong. your type annotations are wrong, and type annotations are part of the code, not comments or something
    • personally i have this issue sometimes, but that's only because i use extremely involved conditional types to get as much type safety as possible. generally to solve this i just give up on narrowing the types that much.
  • "when i use third-party code, some of them may have no typings"
    • i believe typescript just doesn't typecheck those by default? so everything would be typed as any, so this shouldn't give any type errrors.
  • "when i use third-party code, some of them may add conflicting globals"
    • this is a weird one, but again, @ts-ignore.
    • note also that globals are highly not recommended for this exact reason.

on misc. other statements:

  • "statically typed code has no proven benefits over dynamically typed code"
    • this is not what the discussion here is about. if you want to use dynamic typing, that's what javascript is for. this discussion is about typescript and typescript only.
    • this was mentioned because the question was asked, i'm just including this for the sake of posterity

@denoland denoland locked as too heated and limited conversation to collaborators Jul 12, 2021
@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 12, 2021

I am locking the conversation for now. We all appreciate the input, but I think people have stated their feelings, but we are repeating ourselves and not adding any new information.

@lucacasonato
Copy link
Member

lucacasonato commented Apr 21, 2022

Unlocking to gather feedback on the experimental implementation in Deno 1.21 (DENO_FUTURE_CHECK=1 and deno check). Read more about our plans in the 1.21 release blog post: https://deno.com/blog/v1.21#deno-check-and-the-path-to-not-type-checking-by-default. Note type checking is still enabled by default in deno run.

@nayeemrmn
Copy link
Collaborator

This can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.