refactor(read-manifest): improve type safety of manifest validation logic #7345
Conversation
| @@ -9,11 +9,8 @@ export interface WorkspaceManifest { | |||
|
|
|||
| export async function readWorkspaceManifest (dir: string): Promise<WorkspaceManifest | undefined> { | |||
| const manifest = await readManifestRaw(dir) | |||
| if (validateWorkspaceManifest(manifest)) { | |||
There was a problem hiding this comment.
The if statement here was a bit redundant since validateWorkspaceManifest throws an error if the manifest isn't valid.
| @@ -30,11 +27,10 @@ async function readManifestRaw (dir: string): Promise<unknown> { | |||
| } | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| function validateWorkspaceManifest (manifest: any): manifest is WorkspaceManifest | undefined { | |||
| function validateWorkspaceManifest (manifest: unknown): asserts manifest is WorkspaceManifest | undefined { | |||
There was a problem hiding this comment.
TypeScript has 2 kinds of user-defined type guards.
- Predicates that simply return
trueorfalsewithout throwing. - Assertion functions that return void and throw if the assertion fails.
This function is currently written as an in-between. I think the intention was for this to be an Assertion function, which avoids having to return true and an if statement in the calling path.
| @@ -30,11 +27,10 @@ async function readManifestRaw (dir: string): Promise<unknown> { | |||
| } | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| function validateWorkspaceManifest (manifest: any): manifest is WorkspaceManifest | undefined { | |||
There was a problem hiding this comment.
Removing the any in favor of unknown. The later allows us to use TypeScript itself to ensure our validation logic is correct.
|
@JasonMan34 Thanks for the validation logic in #7278! That initial PR was very helpful. 🙂 I had a few suggestions in this PR if you've got time to review. |
|
Fixing the |
Follow up to #7278. I happened to be in this code path while working on #7072 and noticed a few ways the validation logic could be more type safe.
There should be no behavior changes in this PR. Just moving some functions and type annotations around.