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: branded types (2) #62

Merged
merged 27 commits into from
Dec 12, 2023
Merged

refactor: branded types (2) #62

merged 27 commits into from
Dec 12, 2023

Conversation

ComradeVanti
Copy link
Collaborator

This is a copy of #61, but I messed up the commit history on that PR so I did it again with a clean rebase.

Introduce branded types for many of the simple types in the project. This has two primary effects/goals:

  1. This is a good basis to do better validation of cli arguments. This is not implemented in this PR but will be soon.
  2. It exposed a lot of placed in the code where the incorrect type was used. For example name@version was used where only name should have been allowed etc.

Tests were added for all new types.

This PR also includes some helpers for constructing PkgInfo and PkgManifest objects for tests, as well as some other minor changes.

Add branded type for reverse-domain-name. This showed a lot of cases where pkg-name type was used, where reverse-domain-name should have been used. Fixed these cases
Extends project tsconfig, but without emits
Rename to just domain-name since it should also work for regular domain names. Also it's a little shorter that way.

The rules for what makes a valid name also changes a little bit. Now single segment names like just "com" are also valid. We don't need to do that much validation.
Add a builder function for making pkg-info objects for tests.
Includes validation logic and auto-sets derived properties, such as also setting the package _id to the name
Builder does validation and allows for shorter test setups
Fix errors from passing strings to functions expecting domain names. Fix by using name fields from packages or asserting validity using "as"
The response object used by npm-registry-fetch is node-fetch Response, not request Response
Function acts on domain-types and so should be in a different file
Introduce package-url type which represents an url pointing to a package. Also add a few tests
Add type-checking for semantic-version strings. Currently not much validation
Package at specific version
@favoyang favoyang self-assigned this Dec 10, 2023
Copy link
Member

@favoyang favoyang left a comment

Choose a reason for hiding this comment

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

Since you have already defined some primitive type aliases, such as type Registry = string, refactoring to use branded types appears to be quite smooth.

One minor drawback is that, for every branded type, you will need to some helper methods, which makes a branded type feel like a sub-type derived from a primitive type, to which you can add constructor, parser, and validator methods. That makes the code a bit more verbose, but perhaps that is the trade-off you want.

Overall LGTM. See a few minor feedbacks below.

src/types/global.ts Show resolved Hide resolved
src/types/package-id.ts Show resolved Hide resolved
@ComradeVanti
Copy link
Collaborator Author

ComradeVanti commented Dec 12, 2023

which makes a branded type feel like a sub-type derived from a primitive type, to which you can add constructor, parser, and validator methods.

This is actually exactly what I want. For example IpAddress is a subset of string (Every IpAddress is a string, but not every string is an IpAddress). Because of this we do need constructor/validation. This has two benefits:

  • Makes intents more purposful/explicit (Can't use the wrong type etc.)
  • We can reuse these funtions for CLI validation

Copy link
Member

@favoyang favoyang left a comment

Choose a reason for hiding this comment

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

All problems have been resolved. We're ready to merge.

@favoyang
Copy link
Member

Notice that this refactor will not trigger a release by semantic release. Because the commit message is started with "refactor:...". However, your next PR which improves the parameter validation may trigger a release.

@favoyang favoyang merged commit 613168e into openupm:master Dec 12, 2023
2 checks passed
@ComradeVanti
Copy link
Collaborator Author

This is fine. I would actually be very much fine with not having releases until I actually implement some new feature. Less stress in case i break something haha

@ComradeVanti ComradeVanti deleted the branded-types branch December 12, 2023 15:09
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