Skip to content

Turning on strictNullChecks also errors on libraries that haven't turned it on #12615

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

Closed
Vinnl opened this issue Dec 2, 2016 · 6 comments
Closed
Labels
Question An issue which isn't directly actionable in code

Comments

@Vinnl
Copy link

Vinnl commented Dec 2, 2016

TypeScript Version: 2.0.10

When I've turned on strictNullChecks for my project, I get errors on TypeScript projects a depend on that haven't turned that on.

For example, RxJS 5 has not enabled strictNullChecks:

Code

// fromEvent expects the first parameter to be of type EventTargetLike - but, since it's compiled without strictNullChecks, null is valid as well.
Observable.fromEvent(document.getElementById('button'), 'click')

Expected behavior:
The above code to just compile.

Actual behavior:
A compilation error:

    Error TS2345: Argument of type 'HTMLElement | null' is not assignable to parameter of type 'EventTargetLike'.
    Type 'null' is not assignable to type 'EventTargetLike'.

I'm not sure what the best solution is. I don't think they're shipping their own tsconfig, as far as I can see. In any case, that could be problematic as well, if they're using a different version of TypeScript - I guess TypeScript should remain forwards compatible as well.

Perhaps the generated .d.ts files could, during compilation, be modified to add | null to every type to at least ensure compatibility with dependent projects that have enabled strict null checks.

@blakeembrey
Copy link
Contributor

blakeembrey commented Dec 2, 2016

@Vinnl This doesn't look like an issue with the definition, it looks like an issue with your code. Although the library could be updated to support null, does it really support null being used in that position? Looking at the code, it does not: https://github.com/Reactive-Extensions/RxJS/blob/cbc26203dd49398bb3cbd8363122878bc921c865/src/core/linq/observable/fromevent.js#L84-L105. However, the signature for getElementById is getElementById(elementId: string): HTMLElement | null which means null might be returned. Libraries shouldn't be updated to have null in the signature, because that doesn't mean their code is compatible with null.

You can either wrap it in an if statement checking for existence of the element before listening to it, or you can use type coercion to tell the compiler you know it exists.

@Vinnl
Copy link
Author

Vinnl commented Dec 2, 2016

@blakeembrey Actually, I'm using RxJS 5, which does seem to expect it.

I'm inclined to agree that updating libraries to support null might not be a good idea. But then again, when they don't have strictNullChecks enabled, that is basically what they're saying. Although of course, if the type makes it explicit, that sort-of signals to the library consumers that the code has a way of dealing with null values, which it might not. (That would be problematic for libraries without strictNullChecks as well, but at least that wouldn't imply to the consumer that it wouldn't be.)

Thanks for your suggestions though, with those we can make do for now.

@blakeembrey
Copy link
Contributor

Gotcha, thanks for pointing that out. Yeah, it's a tricky situation and it would throw in that case from the library. Not sure if you were planning to handle the catch, but I'd say that catch is there for runtime issues anyway (it's not super useful for type checking to accept null and then throw an error).

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Dec 16, 2016
@mhegazy mhegazy closed this as completed Apr 21, 2017
@Vinnl
Copy link
Author

Vinnl commented Apr 21, 2017

@mhegazy I just noticed this was labelled Question, but it's not really a question, is it? It's a use case TypeScript currently can't properly deal with, so I'm guessing it shouldn't be closed yet (unless this is accepted and won't be fixed).

In fact, I think the use case "libraries using different TSConfig settings than the projects they're used in" is something that might need to be dealt with in general. Effectively, TypeScript with different config settings are almost different languages.

@blakeembrey
Copy link
Contributor

blakeembrey commented Apr 21, 2017

@Vinnl I think you've misunderstood something. This isn't an issue with TypeScript, and it is a question. If you have an issue with the way the definition or library has been written, take it up with the authors (which happen to be RxJS, not TypeScript). It can be handled by TypeScript, but this is purely a definition issue which are not written by the TypeScript team - those are written by module authors.

Edit: I did mention above, though, that passing null would have resulted in an error. The documentation for allowing null in the signature for that seems bad, since it's just moving something the compiler can warn you of into runtime. They could do an overload, I suppose, like (x: null): never, but it wouldn't be super helpful.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 21, 2017

You can see my reply in #14810 (comment) for a similar issue.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants