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

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

Merged
merged 4 commits into from
May 28, 2023

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jan 31, 2022

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 microsoft/TypeScript#46137 (comment)

Should I do the same in @types/ramda as well?

Also I noticed absence of formatter. Don't you mind enabling something like Prettier?

@@ -1013,8 +1013,8 @@ Notes:

*/
// @SINGLE_MARKER
export function endsWith(target: string, iterable: string): boolean;
export function endsWith(target: string): (iterable: string) => boolean;
export function endsWith<T extends string>(target: T, str: string): str is `${string}${T}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed second arg from iterable to str to match startsWith method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we also need to change description of these methods from iterable to something else (input?)

@selfrefactor
Copy link
Owner

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

@zardoy zardoy changed the title Improve startsWith, includes and endsWith typings for for TS 4.5 Improve startsWith, includes and endsWith typings for TS 4.5 Jan 31, 2022
@zardoy zardoy changed the title Improve startsWith, includes and endsWith typings for TS 4.5 feat: Improve startsWith, includes and endsWith typings for TS 4.5 Jan 31, 2022
@selfrefactor
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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.

@zardoy
Copy link
Contributor Author

zardoy commented Feb 23, 2022

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

@selfrefactor
Copy link
Owner

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
Copy link
Owner

I am merging this and it will be released with 8.0

@selfrefactor selfrefactor merged commit b16b36f into selfrefactor:master May 28, 2023
@selfrefactor
Copy link
Owner

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.

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

Successfully merging this pull request may close these issues.

2 participants