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: introduce branded types #1223

Merged
merged 12 commits into from
Jun 13, 2024

Conversation

dammy95
Copy link
Contributor

@dammy95 dammy95 commented Jun 11, 2024

Closes #1141

Created the following branded types:

  • Hostname
  • Token
  • ClientSecret
  • ClientID
  • AuthCode
  • URL (see follow-up question below)

Follow-up question:
While attempting to create the URL branded type, I observed it clashes with JavaScript's URL class. There are also different variations of url (i.e thread_url, html_url, upload_url, etc) in the project that I'm unsure of how to handle – do I ignore these variations and create the branded type for the occurrences with url: string?

@setchy setchy changed the title Enhancement/introduce branded types refactor: introduce branded types Jun 11, 2024
Copy link
Member

@setchy setchy left a comment

Choose a reason for hiding this comment

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

Awesome @dammy95 - left a few comments

src/utils/branded-types.ts Outdated Show resolved Hide resolved
src/utils/branded-types.ts Outdated Show resolved Hide resolved
@setchy
Copy link
Member

setchy commented Jun 11, 2024

Awesome work @dammy95 - thank you 🙏

re: Url - all of those are full html urls... Maybe to avoid the conflict with the JavaScript URL class we can name it differently... perhaps Link or WebUrl. @afonsojramos @bmulholland - what do you guys think?

@bmulholland
Copy link
Collaborator

Link reads less awkwardly than WebUrl, at least to me. I have no better ideas.

@afonsojramos
Copy link
Member

Yep, agree with the above!

@dammy95
Copy link
Contributor Author

dammy95 commented Jun 13, 2024

Thanks for the feedback @setchy @afonsojramos @bmulholland

@setchy setchy modified the milestone: Release 5.9.0 Jun 13, 2024
@dammy95 dammy95 requested a review from setchy June 13, 2024 15:39
Copy link
Member

@setchy setchy left a comment

Choose a reason for hiding this comment

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

Thanks @dammy95 - this is great!

@setchy setchy merged commit d3a4b17 into gitify-app:main Jun 13, 2024
7 checks passed
@setchy setchy modified the milestones: Release 5.9.0, Release 5.8.1 Jun 13, 2024
@setchy setchy added the refactor Refactoring of existing feature label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

introduce branded types
4 participants