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

Use strict mode for internal TypeScript #504

Closed
kitsonk opened this Issue Aug 10, 2018 · 7 comments

Comments

3 participants
@kitsonk
Contributor

kitsonk commented Aug 10, 2018

While it maybe too far of a journey at the moment to force strict mode on end users, it is likely a good idea to ensure that the internal Deno TypeScript is as strict as possible. Enabling --strict enables implicitly --noImplicitAny, --noImplicitThis, --alwaysStrict, --strictNullChecks, --strictFunctionTypes and --strictPropertyInitialization.

The biggest "challenge" is that null and undefined are incompatible with strict mode on, and the Deno TypeScript code is semi loose with this concept. The other challenge is a lot of code uses an assertion pattern for checking, instead of code branching. Currently TypeScript does not have a way of allowing this to affect call flow analysis (see: Microsoft/TypeScript#8655) or co-dependent fields (see: Microsoft/TypeScript#11117), which means that the not null assertion operator (!) has to be used in some cases.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 10, 2018

Collaborator

Yep, I'm all for it.

Collaborator

ry commented Aug 10, 2018

Yep, I'm all for it.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 10, 2018

While it maybe too far of a journey at the moment to force strict mode on end users,

I think Deno should absolutely turn on strict mode for all end users if everyone involved in making Deno agrees that's the right thing to do.

Personally I think it's the right thing to do - I usually turn on strictNullChecks on my code - I think we should do what's right rather than provide a bunch of configuration options.

benjamingr commented Aug 10, 2018

While it maybe too far of a journey at the moment to force strict mode on end users,

I think Deno should absolutely turn on strict mode for all end users if everyone involved in making Deno agrees that's the right thing to do.

Personally I think it's the right thing to do - I usually turn on strictNullChecks on my code - I think we should do what's right rather than provide a bunch of configuration options.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 10, 2018

Collaborator

@benjamingr One of the nice things about dynamic languages is the ability to be messy and fast - I want to make sure that deno doesn't negate that feature.

Collaborator

ry commented Aug 10, 2018

@benjamingr One of the nice things about dynamic languages is the ability to be messy and fast - I want to make sure that deno doesn't negate that feature.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 10, 2018

@ry can you give me an example where strictNullChecks interferes with prototyping? All it does is not let you implicitly assign undefined or null to other types (like number)

benjamingr commented Aug 10, 2018

@ry can you give me an example where strictNullChecks interferes with prototyping? All it does is not let you implicitly assign undefined or null to other types (like number)

@kitsonk

This comment has been minimized.

Show comment
Hide comment
@kitsonk

kitsonk Aug 11, 2018

Contributor

@benjamingr, I beg to differ. There are quite a few use cases that can easily frustrate people who are not familiar with TypeScript. I mentioned two above. Until things like this work out of the box, they will be surprising to users:

declare const foo: Map<string, number>;
if (foo.has("bar")) {
  const value: string = foo.get("bar"); // maybe undefined
}

declare bar: string | undefined;
assert(bar != null);
const value: string = bar; // might be undefined

Both of those impact the internal code of demo, there maybe others. As long as there are valid, common patterns that require the not null assertion (!) then forcing that is a step too far.

Personally I wouldn't be opposed to it being on by default, but I suspect I would lose that argument. It would be nice though if we only allow --strict or loose and not make end users have to understand the nuances.

Contributor

kitsonk commented Aug 11, 2018

@benjamingr, I beg to differ. There are quite a few use cases that can easily frustrate people who are not familiar with TypeScript. I mentioned two above. Until things like this work out of the box, they will be surprising to users:

declare const foo: Map<string, number>;
if (foo.has("bar")) {
  const value: string = foo.get("bar"); // maybe undefined
}

declare bar: string | undefined;
assert(bar != null);
const value: string = bar; // might be undefined

Both of those impact the internal code of demo, there maybe others. As long as there are valid, common patterns that require the not null assertion (!) then forcing that is a step too far.

Personally I wouldn't be opposed to it being on by default, but I suspect I would lose that argument. It would be nice though if we only allow --strict or loose and not make end users have to understand the nuances.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 11, 2018

Personally I wouldn't be opposed to it being on by default, but I suspect I would lose that argument.

The part that's concerning me here is that I get the feeling like personally none of us seem to be too opposed to it being on by default (maybe Ryan is?) but we're all concerned about it.

For what it's worth - I would personally be totally fine and unsurprised with the above - but that might be just personal bias being used to strictNullChecks.

When was the last time you ran much code without strictNullChecks in production (or at all)?

I totally get a REPL not running in strict mode though.

benjamingr commented Aug 11, 2018

Personally I wouldn't be opposed to it being on by default, but I suspect I would lose that argument.

The part that's concerning me here is that I get the feeling like personally none of us seem to be too opposed to it being on by default (maybe Ryan is?) but we're all concerned about it.

For what it's worth - I would personally be totally fine and unsurprised with the above - but that might be just personal bias being used to strictNullChecks.

When was the last time you ran much code without strictNullChecks in production (or at all)?

I totally get a REPL not running in strict mode though.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 11, 2018

Collaborator

We have a long time to make this decision before these semantics are locked down. Thanks for the input - I think we need to experiment with it. We’re not quite at the point where we can configure user tsconfig yet.

Collaborator

ry commented Aug 11, 2018

We have a long time to make this decision before these semantics are locked down. Thanks for the input - I think we need to experiment with it. We’re not quite at the point where we can configure user tsconfig yet.

@ry ry closed this in #505 Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment