Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add "no-null-undefined-union" rule. #4589

Merged
merged 6 commits into from
Mar 27, 2019
Merged

Add "no-null-undefined-union" rule. #4589

merged 6 commits into from
Mar 27, 2019

Conversation

nrathi
Copy link
Contributor

@nrathi nrathi commented Mar 20, 2019

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

CHANGELOG.md entry:

[new-rule] no-null-undefined-union


This change is Reviewable

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @nrathi! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost there

src/rules/noNullUndefinedUnionRule.ts Outdated Show resolved Hide resolved
src/rules/noNullUndefinedUnionRule.ts Outdated Show resolved Hide resolved
adidahiya and others added 2 commits March 27, 2019 09:56
Co-Authored-By: nrathi <neharrathi@gmail.com>
Co-Authored-By: nrathi <neharrathi@gmail.com>
@adidahiya adidahiya merged commit b9e45af into palantir:master Mar 27, 2019
@DarryQueen
Copy link

@nrathi as per this, should we stop using React.ReactNode? At least in 15.6.1, it has a type union of null | undefined.

@rafeememon
Copy link

rafeememon commented Mar 28, 2019

I was pretty surprised with the effect this has on consuming React.ReactNode. In internal discussions, one proposal was to add a NonNullable wrapper, but this seems fishy because in general null is a perfectly reasonable return value for a render function. Further, in most cases, the result of the call is just passed right back into React anyway via another render. Perhaps this rule should only apply to user-land type definitions, and exclude types defined externally? I imagine this issue is not unique to React and is likely to be widespread when consuming other APIs.

@styu
Copy link

styu commented Mar 28, 2019

+1 to @rafeememon ’s point, it feels like the rule should be enforced on type definitions rather than on consumed types, so you wouldn’t be able to define types with the union, but could still consume types that have defined the union already. That way you can trend towards the right direction without encouraging the disabling of this rule due to external types

@adidahiya
Copy link
Contributor

@styu that makes sense. I wouldn't recommend enabling this rule right away across the board. I filed #4617

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

Successfully merging this pull request may close these issues.

7 participants