rambda
rambda copied to clipboard
feat: Improve `startsWith`, `includes` and `endsWith` typings for TS 4.5
There might be a better title for this PR.
Starting from TypeScript 4.5 strings can be narrowed: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#template-string-types-as-discriminants
Also see https://github.com/microsoft/TypeScript/pull/46137#issuecomment-932730666
Should I do the same in @types/ramda
as well?
Also I noticed absence of formatter. Don't you mind enabling something like Prettier?
will take a look at it, but there is linting enabled, but it is missing in CONTRIBUTING.md and it will be updated.
FIrst, I want to say sorry for my late reply.
Second, I see few issues arising from this MR:
- we'll increase the minimum TS version which works with latest Rambda version
- I don't mind increasing TS version, if the value is there. Having said that, I'd expect the new typings to return
true/false
instead ofboolean
. I see from my check and from your typings test, that return value remains unchanged. Then what will be the benefit of the change?
ok, I see in:
if(result){
iterable // $ExpectType 'foo bar'
}
the actual value of this MR, but what about the else
:
const result = endsWith(target, iterable)
if(result){
iterable // $ExpectType 'foo bar'
}else{
// Property 'length' does not exist on type 'never'
iterable.length // $ExpectType 'foo bar'
}
This is very new TS feature and I believe that TS team will build upon it. Then the value of your suggested change might increase, but currently I am inclined towards closing this MR. Let me know your thoughts on the matter.
One more note.
Such typings:
export function endsWith<Full extends string, CheckEnd extends string>(target: CheckEnd, str: Full): string extends Full | CheckEnd
? boolean
: Full extends `${string}${CheckEnd}`
? true
: false
gives us true/false
as return value. But again, I'd wait for few more TS versions before using template literal feature.
By the way, are you sure that minimum supported TS version should be raised only because of this change? Though I didn't test it with previous versions.
but what about the else
Oh wow, this is pretty interesting, I'm gonna look into it tomorrow. But for now, if you think this is too early to add here, I'm fine with closing this PR. I believe we could reopen it some day (if this is really problem with just TS).
There is a test in rambda-scripts
repo, that tries to find the first version of TS which works with Rambda. I am not fully sure that the change will increase the TS requirement, but it is possible. I'll run a check and will get back to you. On second thought, I like endsWith
that returns true/false
, so if you are willing to make appropriate changes, then I'd be more in favor of this change even with the price of higher TS version requirement. FYI, the example code of endsWith
is taken from https://github.com/DetachHead/ts-helpers/blob/master/src/utilityTypes/String.ts
I am merging this and it will be released with 8.0
Actually, I cannot make typing test give me proper result, so I will decline it. If you provide typing test for this, I am willing to approve it.