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

refactor: nominal typing #2021

Closed
wants to merge 6 commits into from
Closed

refactor: nominal typing #2021

wants to merge 6 commits into from

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented May 31, 2021

note: this is marked as draft to ensure that the design is sound before fixing up the tests/methods in the controller layer

Problem

Currently, our types for email and mobile phone number are string - this results in possibly incorrect definitions where we want to use a Email type but we pass in a normal string, which is not caught by the type system.

a more detailed discussion can be found here!

Solution

  1. use ts-brand to get pseudo nominal typing through the use of a brand.
  2. create a companion object with the same name as the type so that conversion to/from the type is painless

Design Considerations

  1. zod was not used due to its limitations - z.infer associates input types with output types of the schema. Hence, even though we validate it, the output is still string, which doesn't help.

@seaerchin seaerchin requested a review from karrui May 31, 2021 04:08
@seaerchin seaerchin marked this pull request as draft May 31, 2021 04:08
@@ -0,0 +1,65 @@
import { identity } from 'lodash'
Copy link
Contributor

@karrui karrui May 31, 2021

Choose a reason for hiding this comment

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

i think you are overcomplicating this. why can't we just use type-fest's Opaque type instead of a relatively unused and not-updated library?

to create an opaque type, we just need a creator method, e.g.

import { Opaque } from 'type-fest'
type Email = Opaque<string, 'Email'>
const createEmail = (email: string): Email => {
  // Can even add validation here and throw error or something
  return email as Email;
}

you really don't need something so complex like assert, etc.

everything else should stick to the same doesn't it? Only functions that absolutely need the correct type to declare it

Copy link
Contributor Author

@seaerchin seaerchin May 31, 2021

Choose a reason for hiding this comment

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

the reason why we can't use the Opaque type is because it's meant as a black box - once we use it, there's no way for us to extract the type out from Opaque because it's set as Type & {readonly __opaque__: Token} where there isn't a witness to the base type.

wrt assert and extract i just wanted to add utility methods for ppl to use AHAH so that the api is sensible and easy to use

Copy link
Contributor

@karrui karrui May 31, 2021

Choose a reason for hiding this comment

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

we should not need to extract the type out. any arguments of 'string' (for example) will be able to be used with Opaque<string, 'Key> since the Opaque type is a superset.

@@ -434,15 +432,21 @@ export class MailService {
formData,
}: {
replyToEmails?: string[]
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also be of Email[] type

@seaerchin seaerchin closed this Jun 8, 2021
@karrui karrui deleted the refactor/nominal-typing branch August 17, 2021 02:44
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