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

[misc] typescript: update typescript to 2.0.8, add strictNullChecks=true #3591

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
8 participants
@tiagoefmoraes
Contributor

tiagoefmoraes commented Nov 10, 2016

…rSpec type #3589

@jsf-clabot

This comment has been minimized.

jsf-clabot commented Nov 10, 2016

CLA assistant check
All committers have signed the CLA.

@horiuchi

This comment has been minimized.

horiuchi commented Nov 11, 2016

I also need this changing!

@afc163

This comment has been minimized.

Contributor

afc163 commented Nov 11, 2016

+1

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

Can you please update the typing tests in a way that would have failed this, please.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

Also this strict null checks - isnt this possibly causing multiple problems for all existing definitions for which null was assumed possible. @blakeembrey can you chim in, please.

@blakeembrey

This comment has been minimized.

Contributor

blakeembrey commented Nov 12, 2016

@ichernev This is a good change, I had noted to the TypeScript team that this would happen during the 2.0 beta releases but it hasn't been fixed. It a real pain as a consumer and author. strictNullChecks won't cause any issues for the consumers, but the use of the undefined type does make the definition 2.0+ now (technically a breaking change). It might be a good idea to update anywhere else, sometime in the future, with null and undefined types for better type checking (people without strictNullChecks just see null and undefined disappear in the signature).

See Microsoft/TypeScript#9235.

Edit: Changed "in" to "during" in case that was the source of confusion.

@horiuchi

This comment has been minimized.

horiuchi commented Nov 12, 2016

@blakeembrey TypeScript 2.0 is already released: https://blogs.msdn.microsoft.com/typescript/2016/09/22/announcing-typescript-2-0/
That is not beta version. 😄

@blakeembrey

This comment has been minimized.

Contributor

blakeembrey commented Nov 12, 2016

@horiuchi I didn't say it was, I think you mis-read my comment. Also, that's my name in the post, so I am aware of it 😄 I was pointing people to an issue I logged during the TS 1.9 nightlies (thought it was in the 2.0 betas, but it was even earlier) that brought up the potential for this issue to be a PITA for all module authors.

@horiuchi

This comment has been minimized.

horiuchi commented Nov 12, 2016

@blakeembrey Oh, sorry. I was misunderstanding. 😢
I was also using the TS of beta version 😄

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

@blakeembrey so 1) this undefined is a special case because of the []. 2) strictNullChecks is not that strict, but why use it then and 3) in the future add null/undefined if its expected in the signatures but it wont hurt if we dont add it... how is this strict. And how would we know where to add (like will there be issues or it will just help people figure out where null is ok)

@blakeembrey

This comment has been minimized.

Contributor

blakeembrey commented Nov 12, 2016

@ichernev It's strict for the developer, but not so useful as an author because there's no code in the definition to tell if it should be null or not for strictNullChecks to be useful. The index signature issue is the only place I'm aware of that can cause a difference in compilation for a consumer. Basically, typing null in the right places as the definition author is the same as typing string or number - it comes from an understanding of the library itself.

On the consumer side, and I may not have described it properly above, it will give a signature like string | undefined, for example. However, that will only be when the consumer is also using strictNullChecks. In this case, it is actually really useful because consumers can avoid bugs like x.substr where x is string | undefined and they would be prompted to make sure x exists before treating it as a string.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

Also about backwards compatibility -- should we ship the old and the new code somehow? Wouldn't we get hundreds of angry pre 2.0 typescript users that say our d.ts is wrong? In general, did you plan for a transition? What if we just don't upgrade to 2.0? Is it a problem for 2.0 users? That is a bit disappointing ... (if true)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

So its more important to say which of our methods can return null/undefined, so the strict checking will warn the users. What we take doesn't matter for strictNullChecks.

@blakeembrey

This comment has been minimized.

Contributor

blakeembrey commented Nov 12, 2016

Basically, it's really useful to a lot of people using the stricter subset of TypeScript to add stricter signatures to catch bugs.

On backward compatibility, you can't really ship them together unfortunately. In this signature, you may be able to use | void as a workaround (I haven't tested this, but void <> undefined), but I'd recommend adopting 2.0 syntaxes where possible as it's really just a PITA to maintain otherwise and you won't be able to use the new things (like null | undefined types). In general, I've just been updating with the latest TypeScript as I go and haven't seen any issues with consumers being stuck on lower TypeScript versions.

So its more important to say which of our methods can return null/undefined, so the strict checking will warn the users.

Both can be useful. For consumers who enable strictNullChecking, it can be helpful on input types too (E.g. callbacks accepting null). Here's an example that I did (used void but can since change to null with 2.0) - types/npm-generic-pool#3 - basically I wanted to be able to cb(undefined, value) but with strict null checking I wouldn't be able to use undefined as the first argument.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

I understood, thank you for the explanation. So from a backwards compatibility standpoint -- it should be painless and we'll encourage users to upgrade.

basically I wanted to be able to cb(undefined, value) but with strict null checking I wouldn't be able to use undefined as the first argument.

Well doesn't that mean that, for example in our moment constructor, moment(null) is a valid way to create an invalid moment, but because its not explicitly specified now you won't be able to do it? That means a ton of PRs for | null here and there ... I mean I'd prefer to fix them all at once and not spend time sifting through a sea of incomplete PRs.

@blakeembrey

This comment has been minimized.

Contributor

blakeembrey commented Nov 13, 2016

@ichernev Yep, anyone using strictNullChecks won't be able to do moment(null) right now. If you want to enable this, sounds like a good idea to go through and improve it everywhere in one swoop.

@tiagoefmoraes

This comment has been minimized.

Contributor

tiagoefmoraes commented Nov 14, 2016

@ichernev added some tests, but what I think really does the check is the strictNullChecks. Maybe @blakeembrey knows a better way to test this.

@bcherny

This comment has been minimized.

bcherny commented Nov 17, 2016

Bump - @ichernev do you feel comfortable merging this? We're locking down our Moment to an old version for now.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 22, 2016

Merged in c891a4f

@ichernev ichernev changed the title from update typescript to 2.0.8, add strictNullChecks=true and fix Calenda… to [misc] typescript: update typescript to 2.0.8, add strictNullChecks=true Nov 22, 2016

@ichernev ichernev closed this Nov 22, 2016

ichernev added a commit that referenced this pull request Nov 22, 2016

Merge pull request #3591 from tiagoefmoraes:fix_3589
[misc] typescript: update typescript to 2.0.8, add strictNullChecks=true

@mj1856 mj1856 added this to the 2.17.0 milestone Jan 17, 2017

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