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: move @babel/helper-validator-option to ts #12410

Merged
merged 3 commits into from Dec 15, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 27, 2020

Q                       A
License MIT

@JLHwung JLHwung added the Flow -> TS Tracking repository migration from Flow to TS label Nov 27, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 81b34ac:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 27, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/35001/

@JLHwung JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Nov 27, 2020
@JLHwung JLHwung force-pushed the helper-validator-option-ts branch 2 times, most recently from a32b895 to 12473ca Compare December 1, 2020 14:45
@@ -46,13 +45,13 @@ export class OptionValidator {
return value;
}

validateStringOption(
validateStringOption<T>(
name: string,
value?: string,
Copy link
Member

Choose a reason for hiding this comment

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

Actually, value could be anything here I think (otherwise we wouldn't need this helper).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally value should be of type that extends string. So if we accidentally pass number value to validateStringOption, the type checker should throw.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't it be anything, since it's whatever the user gave us as input? We cannot statically know the type of plugin options, so an user could pass a number to a plugin, the plugin will pass a number to this helper, and then this helper will throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also offer our type interface to programmatic users:

{ a: any, b: any }

is technically correct (which is the type user inputs) but not much of value for programmatic usage (API caller) and/or schema check. The validator here is a runtime enforcement of type checking -- so the user input does match our assumption of types, so it makes sense that if the type checking is good, it is also sound in runtime.

@nicolo-ribaudo nicolo-ribaudo removed the PR: Internal 🏠 A type of pull request used for our changelog categories label Dec 10, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 73f3032 into babel:main Dec 15, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the helper-validator-option-ts branch December 15, 2020 10:56
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Flow -> TS Tracking repository migration from Flow to TS outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants