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

Enable strictNullChecks compiler option for all packages #325

Open
giladgray opened this issue Dec 6, 2016 · 9 comments
Open

Enable strictNullChecks compiler option for all packages #325

giladgray opened this issue Dec 6, 2016 · 9 comments

Comments

@giladgray
Copy link
Contributor

I think enabling strictNullChecks compiler option for datetime package will help us detect and prevent a whole host of potential errors, such as #322. it'll also address #274 by making the typings more accurate (as explicit null Dates actually means something in the component).

@adidahiya
Copy link
Contributor

I think the compiler option should be enabled for all packages. Updating issue title to reflect this.

@adidahiya adidahiya changed the title Enable strictNullChecks for datetime package Enable strictNullChecks compiler option for all packages Dec 7, 2016
@giladgray
Copy link
Contributor Author

giladgray commented Dec 7, 2016

Fair enough! Converting core will take some time due to its sheer size, hence why I started with datetime. Any idea if this will affect typings in a breaking way?

@adidahiya
Copy link
Contributor

No, any changes in the generated .d.ts files ought to be tractable & compatible with consumers' tsc versions. If anything, it will expose bugs in consuming applications (not handling null / undefined values).

@giladgray
Copy link
Contributor Author

in my ideal world, we also enable the no-null-keyword lint rule and any explicit usage of null would have to be whitelisted

#419 (comment)

@giladgray
Copy link
Contributor Author

giladgray commented Jan 17, 2017

started working on this and quickly ran into some heavy sadness with setState:

basically, this.state fields should all be required (otherwise you have to cast/check before using any as they're all X | undefined), but then we can't setState() with partial objects. the solution is Pick and @ericanderson has submitted a PR to DT to update react typings accordingly: DefinitelyTyped/DefinitelyTyped#13155 (blocked on this).

@adidahiya
Copy link
Contributor

Cool, it looks like that PR will be merged in DefinitelyTyped soon enough and I don't mind blocking on it.

@adidahiya
Copy link
Contributor

We're good to go on the setState enhancements in the React typings now. Pull the latest @types/react.

@brieb brieb mentioned this issue Sep 14, 2017
2 tasks
@brieb
Copy link
Contributor

brieb commented Sep 15, 2017

If we forbid using null, what's the best way to indicate an empty value for controlled input components. Using undefined doesn't work because that indicates that the component should be in uncontrolled mode. Some inputs have a logical empty value - like for text inputs, the empty value should be represented as "". But, what about for something less clear - like a date input. Should we export an explicit empty value to use in that case? Looks like we do something that here https://github.com/palantir/blueprint/blob/45c07c597c716655a028f092e9966cc65b54b15f/packages/core/src/components/forms/numericInput.tsx

@adidahiya
Copy link
Contributor

(updated your link to be a permalink)

Should we export an explicit empty value to use in that case?

yeah, this sounds reasonable to me. I would be curious to see how far we can get with avoiding null in our public API.

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

No branches or pull requests

3 participants