fix: handle non-literal strings #80

jly36963 · 2023-10-12T00:48:22Z

Description

Handle literal and string types differently:

Current behavior: string-ts types handle string literal type inputs correctly, but return incorrect types if the input type is string
Desired behavior: correctly handle string type input

jly36963 · 2023-10-12T00:48:43Z

Related to: #79

@ArthurKa is this what you were describing?

Non-literal string has bad type:

Asserting that length of string is number:

I started with a fix for Length, I'm going to add tests wherever missing to expose any similar issues

jly36963 · 2023-10-12T05:39:55Z

src/internal/intrinsic.ts

@@ -0,0 +1,15 @@
+export type UncapitalizeSTS<T extends string> = string extends T


Unfortunately this file is necessary.

The built-in Uncapitalize/Lowercase/Uppercase/Capitalize types have what (IMO) is bad behavior

type CapitalizeSTS1 = Expect<Equal<CapitalizeSTS<'abc'>, 'Abc'>> type CapitalizeSTS2 = Expect<Equal<CapitalizeSTS<string>, string>> type bad1 = Expect<Equal<Capitalize<'abc'>, 'Abc'>> type bad2 = Expect<Equal<Capitalize<string>, Capitalize<string>>> // ^ WTF

The bad2 type test showcases the "bad behavior" around the built-in types.

Why doesn't Capitalize<string> resolve to string?

This unfortunate behavior affects everything downstream (ie: word casing and object keys). We need utilities that work on both string and literal types, so I had to replace the built-in casing types with new types that would work correctly for both string and literal type inputs.

I don't know the answer but that makes me feel better about our functions also not being prepared to handle loose types

jly36963 · 2023-10-12T05:43:20Z

src/internal/internals.test.ts

    Equal<
      Subject.Reject<['one', '', 'two', '', 'three'], ''>,
      ['one', 'two', 'three']
    >
  >
+  // // TODO: fix


Okay with leaving this one as unresolved for now, as Reject is not exported

Sure, and also I'm ok about all tests in this file as they don't have runtime counterparts

gustavoguichard · 2023-10-12T09:10:18Z

src/native/char-at.test.ts

+  type test3 = Expect<Equal<CharAt<'some nice string', number>, string>>
+
+  // TODO: index greater than Length<T>
+  // type test4 = Expect<Equal<CharAt<'some nice string', 100>, ''>>


This is gonna return undefined which is equal to runtime

Suggested change

// type test4 = Expect<Equal<CharAt<'some nice string', 100>, ''>>

// type test4 = Expect<Equal<CharAt<'some nice string', 100>, undefined>>

That's what I thought as well, but:

gustavoguichard · 2023-10-12T09:11:14Z

src/internal/math.test.ts


  type testGetPositiveIndex1 = Expect<
    Equal<Math.GetPositiveIndex<'abc', -1>, 2>
  >
+  type testGetPositiveIndex2 = Expect<


I would be ok with leaving these methods simpler since they are only used by us internally

gustavoguichard · 2023-10-12T09:11:59Z

src/internal/intrinsic.ts

@@ -0,0 +1,15 @@
+export type UncapitalizeSTS<T extends string> = string extends T


I don't know the answer but that makes me feel better about our functions also not being prepared to handle loose types

gustavoguichard · 2023-10-12T09:19:12Z

src/utils/uncapitalize.ts

-export function uncapitalize<T extends string>(str: T): Uncapitalize<T> {
+export function uncapitalize<T extends string>(str: T): UncapitalizeSTS<T> {


We will have to export these type utilities as well :/

I can update (to add these exports) during lunch today -- let me know if you have a preference for names. I chose STS as an arbitrary suffix, and I'm open to suggestions

@p9f any opinions here? Maybe we should leave the changes in these functions to another PR so we can discuss.
If we export these types we must think through, if we ship with different types we are gonna be breaking a concept of matching type and runtime.
I'm thinking if there's a way to make our type utilities behave more like those of TS and leave them unchanged?

if we ship with different types we are gonna be breaking a concept of matching type and runtime.

i am not sure to understand this part :/ afaiu this whole PR is making the type more precise for non literal string - but where do we have an inconsistency between runtime and types?

I don't think our types will work with string types (eg: CamelCase<string>) without fixing the bad behavior that TS built-in functions (eg: Uppercase) have.

Any utility that uses a built-in func will have a return type like Uppercase<string> instead of string. This is weird. Also, we pipe types in a lot of places, so it will cause issues where the outputs/inputs don't align between piped types

I understand it now.. I'll think through it.
Still not convinced the value is worth the added LOCs and complexity 😅

IMO it is a critical mistake to not handle string -- and the intrinsic/built-in string utilities cannot do that.

Uncapitalize docstring: "Convert first character of string literal type to lowercase"
This type is only meant to be used with literals. Same with the others.

jly36963 · 2023-10-21T13:47:25Z

Given a scenario where string is used as an input to a type utility:

Length<string> // 1

Often times we are getting an incorrect literal type as the output.
The existing type tests only consider the literal case, not string.

Possible solutions:

Current behavior: return (wrong) literal type
PR behavior: return less-specific type (often string or number) when string is used
- Eg: Length<string> // number
return never
- Eg: Length<string> // never: we only support literals
Other solution

Regardless of the solution, I don't think that leaving the current behavior in place is acceptable.
IIRC, 2/3 of the type utilities will return a wrong type if a non-literal is used.

fix: handle non-literal strings

96758b8

jly36963 added 5 commits October 11, 2023 19:01

Internals

4ac721c

Fixes

ac553ca

Fixes

ef56635

Fixes

207e409

Fixes

ab7d700

jly36963 requested a review from gustavoguichard October 12, 2023 03:21

jly36963 added 3 commits October 11, 2023 23:30

Fixes

06b990c

Fixes

026cd06

Fixes

7ee208d

jly36963 commented Oct 12, 2023

View reviewed changes

Fixes

1436ec2

jly36963 commented Oct 12, 2023

View reviewed changes

jly36963 marked this pull request as ready for review October 12, 2023 05:44

jly36963 added 2 commits October 12, 2023 05:49

Simplify

77c6ab2

cleanup

516f2d8

gustavoguichard reviewed Oct 12, 2023

View reviewed changes

jly36963 added 3 commits October 12, 2023 18:03

Rename

5270ff7

Cleanup

15e6e12

oops

779386c

jly36963 marked this pull request as draft October 13, 2023 12:34

readme

ed22848

jly36963 marked this pull request as ready for review October 14, 2023 13:38

jly36963 closed this Oct 21, 2023

jly36963 mentioned this pull request Nov 2, 2023

fix: handle non-literal strings #105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle non-literal strings #80

fix: handle non-literal strings #80

jly36963 commented Oct 12, 2023 •

edited

Loading

jly36963 commented Oct 12, 2023 •

edited

Loading

jly36963 Oct 12, 2023 •

edited

Loading

gustavoguichard Oct 12, 2023

jly36963 Oct 12, 2023 •

edited

Loading

gustavoguichard Oct 12, 2023

gustavoguichard Oct 12, 2023

jly36963 Oct 12, 2023

gustavoguichard Oct 12, 2023

gustavoguichard Oct 12, 2023

gustavoguichard Oct 12, 2023

jly36963 Oct 12, 2023 •

edited

Loading

gustavoguichard Oct 12, 2023

p9f Oct 12, 2023

jly36963 Oct 12, 2023

gustavoguichard Oct 12, 2023

jly36963 Oct 13, 2023 •

edited

Loading

jly36963 commented Oct 21, 2023

		@@ -0,0 +1,15 @@
		export type UncapitalizeSTS<T extends string> = string extends T

	// type test4 = Expect<Equal<CharAt<'some nice string', 100>, ''>>
	// type test4 = Expect<Equal<CharAt<'some nice string', 100>, undefined>>

		export function uncapitalize<T extends string>(str: T): Uncapitalize<T> {
		export function uncapitalize<T extends string>(str: T): UncapitalizeSTS<T> {

fix: handle non-literal strings #80

fix: handle non-literal strings #80

Conversation

jly36963 commented Oct 12, 2023 • edited Loading

Description

jly36963 commented Oct 12, 2023 • edited Loading

jly36963 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jly36963 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jly36963 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jly36963 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

jly36963 commented Oct 21, 2023

jly36963 commented Oct 12, 2023 •

edited

Loading

jly36963 commented Oct 12, 2023 •

edited

Loading

jly36963 Oct 12, 2023 •

edited

Loading

jly36963 Oct 12, 2023 •

edited

Loading

jly36963 Oct 12, 2023 •

edited

Loading

jly36963 Oct 13, 2023 •

edited

Loading