Fix type inference for first() and last() #2001
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Because of the way types are inferred in both TypeScript and Flow, it's possible for the
NSV
(Not-set value) of thefirst()
andlast()
methods to be inferred incorrectly. For example:This is quite subtle and it's easy for it to go unnoticed (for example, this went unnoticed for several years in our company's moderately large TypeScript codebase with ~2000 TypeScript files and ~200,000 lines of code). But it can cause bugs because it makes it easy to accidentally ignore the fact that the list may be empty.
Fix
This PR adds type tests for the above case, and fixes the types by updating
first()
andlast()
to use method overloads for the no-argument case. Note that this makes the types the same as theget()
method, which already uses a method overload and doesn't suffer from this problem.Technically this is a breaking change to the types: if this change is released, people will likely get some type errors when they upgrade to the version with the fix. However, those should all be places where there is a potential bug due to ignoring the possibility of the list being empty. If people want to ignore the error, they can do so using a
!
assertion (e.g..first()!
).Prior art
first()
/last()
types that the current PR does: Add separate types for Collection first()/last() with zero arguments #1653.get(0)
as a temporary workaround, meaning even at that time the types forget()
were correct. There was never a reply to this comment, so it may have simply been unnoticed.first()
andlast()
, but did not fix the issue described above.