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 start with types #986

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aadito123
Copy link

Changes the output type for startsWith and endsWith to actually reflect.

Previously:
startsWith("abc") -- output --> string
Now:
startsWith("abc") -- output --> abc${string}

Similarly with endsWith.

@fabian-hiller
Copy link
Owner

Thanks for creating this PR. The idea is great. Right now startsWith and endsWith are just validation actions and don't transform their output type. This would force us to change .kind from 'validation' to 'transformation', but that feels wrong somehow because the main use case is the validation. We could also allow validation actions to transform their output type in such cases. For me, a strong mental model is very important. So it's important for me to make the right decision in this case.

Issue #945 is related.

@fabian-hiller fabian-hiller self-assigned this Dec 19, 2024
@fabian-hiller fabian-hiller added enhancement New feature or request question Further information is requested priority This has priority labels Dec 19, 2024
@aadito123
Copy link
Author

Fixed an issue to stop piping the TInput.
For example, pipe(string(), startsWith('hi'), startsWith('bye')), would result in an invalid type of byehi${string}.
This would fail validation so there won't be any runtime consequences, and also, breaks the following case:
pipe(string(), startsWith('hi'), endsWith('bye')) since it would give a type ${string}bye instead of hi${string}bye.

I think I align with validation transforming the output types, even if the runtime value is unaffected. I believe the validation should serve as a way of effectively narrowing the type. The input for the parse methods already transform the input type, since they can take unknown typed inputs, so further narrowing the type via these methods seems consistent to me.

@fabian-hiller
Copy link
Owner

Thank you for your feedback. I will further investigate it in the next few weeks.

@fabian-hiller
Copy link
Owner

I though about this change and noticed a big problem. Have a look at the following example:

const Schema = v.pipe(v.string(), v.startsWith('foo@'), v.email());

If the startsWith validation fails with the current implementation, the output is still a string and therefore valid. Therefore, the email action can still be executed. If we were to change the implementation so that startsWith transforms its output type, we would be forced to mark the output of startsWith as untyped, since the output type cannot be guaranteed. This forces the pipe to stop its execution as subsequent actions may require this type.

@aadito123
Copy link
Author

I am not sure I understand what you mean. My PR does not make any runtime changes to the validation. Why would narrowing the output type force us to mark the output as untyped? Furthermore, even if subsequent actions require the type, the input type on the validation actions in pipe should always be the type defined by the schema, and not the previous validation. Therefore, it should not rely on the output type of previous validation. I do understand why that behaviour may not be intuitive.

Is there any way we can have some ability to opt into this more accurate narrowing?

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 24, 2024

The problem is this. The data type of a pipeline item is based on the previous item. So in the following example, the input type of email is based on the output type of startsWith. Currently, startsWith does not change its output type. Even if startsWith fails, we still know that the output will be a string and email can be executed.

const Schema = v.pipe(v.string(), v.startsWith('foo@'), v.email());

But if we change the output type of startsWith from string to `${TRequirement}${string}` the input type of email will be based on this type. If startsWith fails, we know that the output value does not match the output type. Therefore, we can't perform any subsequent action that is based on that type.

In simple words: If startsWith fails, we can't perform any subsequent action that expects the input to match the `${TRequirement}${string}` type because this may result in a runtime error.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 24, 2024

At the moment I see one possible workaround and one solution.

The workaround would be to manually transform to the expected type. This works because a transformation is only executed if there was no issue before. So whenever transform is executed, you know that startsWith was successful.

const Schema = v.pipe(
  v.string(),
  v.startsWith('foo@'),
  v.transform((input) => input as `foo@${string}`)
);

A possible solution would be to provide a startsWith2 transformation action (the name is just an example) that not only checks for the start of the string, but also transforms the output type to `${TRequirement}${string}` if successful.

Due to the type safety of this library, there is a necessary and important difference in the behavior of validation actions and transformation actions.

@sandros94
Copy link

Ah, I do start to understand the issue now.

Although it is not fully clear why we are forced to mark the output as untyped instead of the last successful validation step (in example caching it until the current validation is successful)

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 25, 2024

I agree. This is probably easy to solve using pure JavaScript, but I am not sure how to make it type safe using TypeScript. 😐

I am happy to get more feedback, also from others, but in the meantime, this is something we will probably reconsider after the release of v1.

@fabian-hiller fabian-hiller added next version Something to look at for our next major release and removed priority This has priority labels Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next version Something to look at for our next major release question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants