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!: parsed optionals and enum type safety #174

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

Conversation

BobbieGoede
Copy link

@BobbieGoede BobbieGoede commented Dec 11, 2024

I tried to make this PR as small as possible, these changes will cause type errors in projects that have relied on the parsed arguments never being undefined. I'm not very proficient with advanced types, and may have made obvious mistakes!

This PR changes the types to be more accurate:

  • Parsed enum arguments resolve to a union type of its configured option
  • Optional arguments (required: false, default: undefined) correctly resolve to undefined
defineCommand({
  args: {
    myEnum: { type: "enum", options: ["foo", "bar"] },
    myOptionalArg: { type: "string", required: false },
    myOptionalArgWithDefault: { type: "string", default: "my-default" },
  },
  run({ args }) {
    args.myEnum // "foo" | "bar" | undefined
    args.myOptionalArg // string | undefined
    args.myOptionalArgWithDefault // string
  }
})

These type changes make use of defineCommand<const T extends ArgsDef = ArgsDef> (note the addition of const), I'm not sure if this has other implications but it seems to be required for the enum inference.

While I was working on this I was looking to see if an issue exists about this type behavior but I couldn't find any, after already having a working solution I found that #132 targets some of the same issues in a similar way, I took it as an example to restructure the ParsedArgs type.

@pi0
Copy link
Member

pi0 commented Dec 11, 2024

I appreciate the work you put here ❤️

To be honest I prefer to have one mode, if we go strict, always strict (perhaps in a major) -- but keep simplicity and consistency first.

Also please consider that enum support is going under heavy refactors that will be a "subtype", this will likely conflict with your PR.

For now setting PR on draft.

@pi0 pi0 marked this pull request as draft December 11, 2024 12:10
@BobbieGoede
Copy link
Author

Thanks for giving it a look! I prefer one mode as well, I'll remove the conditionals it should be much simpler/cleaner without.

Also please consider that enum support is going under heavy refactors that will be a "subtype" ...

I'm not entirely sure what you have in mind for enum as a subtype 😅 If this is something you prefer to implement yourself (or if explaining the change is time consuming) I understand, otherwise a reference to a similar functionality/project or pseudo code example could be enough for me (or someone else) to take a shot at it.

..., this will likely conflict with your PR.

Not an issue 🙏 the scope and scale of this project is nice, features/fixes like these are good exercise.

@BobbieGoede BobbieGoede changed the title feat: opt-in type safety feat: parsed optionals and enum type safety Dec 11, 2024
@BobbieGoede BobbieGoede changed the title feat: parsed optionals and enum type safety feat!: parsed optionals and enum type safety Dec 11, 2024
@BobbieGoede
Copy link
Author

I have removed the opt-in functionality and types and updated the PR title and description (it's a breaking change).

Just realized that the types can still be improved further to also cover argument aliases, so I will look into this as well.

@BobbieGoede
Copy link
Author

The aliases are now on the resolved args object with the correct types as well, I'm not sure if this is something we actually want to support though 🤔 since the value can be retrieved from its normal property name, but it would be accurate since the value is present on the alias property as well.

Also I'm not sure how to keep the formatting looking nice with types like these, I added prettier-ignore statements but I didn't see these elsewhere in the codebase, perhaps these should be removed.

@BobbieGoede BobbieGoede marked this pull request as ready for review December 11, 2024 14:41
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