rambda icon indicating copy to clipboard operation
rambda copied to clipboard

feat: Improve `startsWith`, `includes` and `endsWith` typings for TS 4.5

Open zardoy opened this issue 2 years ago • 6 comments

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?

zardoy avatar Jan 31 '22 11:01 zardoy

will take a look at it, but there is linting enabled, but it is missing in CONTRIBUTING.md and it will be updated.

selfrefactor avatar Jan 31 '22 12:01 selfrefactor

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 of boolean. I see from my check and from your typings test, that return value remains unchanged. Then what will be the benefit of the change?

selfrefactor avatar Feb 23 '22 23:02 selfrefactor

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.

selfrefactor avatar Feb 23 '22 23:02 selfrefactor

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.

selfrefactor avatar Feb 23 '22 23:02 selfrefactor

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).

zardoy avatar Feb 23 '22 23:02 zardoy

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

selfrefactor avatar Feb 23 '22 23:02 selfrefactor

I am merging this and it will be released with 8.0

selfrefactor avatar May 28 '23 22:05 selfrefactor

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.

selfrefactor avatar May 31 '23 22:05 selfrefactor