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: add @babel/helper-validator-option #12006

Merged
merged 10 commits into from Sep 24, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Aug 25, 2020

Q                       A
Tests Added + Pass? Yes
Any Dependency Changes? A new package @babel/helper-validator-option is drafted to replace both invariant and levenary
License MIT

This PR is meant to resolve​ #10927 (comment) and tanhauhau/levenary#7 (comment).

#10927 will be rebased on this PR to apply similar option checks for all other @babel/presets and related @babel/plugins.

@JLHwung JLHwung added PR: Polish 💅 A type of pull request used for our changelog categories pkg: preset-env labels Aug 25, 2020
ModulesOption[modulesOpt.toString()] || modulesOpt === ModulesOption.false,
`Invalid Option: The 'modules' option must be one of \n` +
`The 'modules' option must be one of \n` +
Copy link
Contributor Author

@JLHwung JLHwung Aug 25, 2020

Choose a reason for hiding this comment

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

These errors will now be thrown with @babel/preset-env: prefix.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 25, 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 82a0546:

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

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 25, 2020

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

}
}

// note: we do not consider rewrite them to high order functions
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this comment, but if we want to make it clear that this should be refactored in the future maybe we can add a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact it is quite opposite: It is to leave it as-is (for less call stacks when error is thrown) unless we are going to support number-only preset options in the future, which is quite unlikely.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@existentialism
Copy link
Member

existentialism commented Aug 27, 2020

Nit, but should we call this option-validator?

EDIT: I guess it's to be consistent with validator-identifier, nbd.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

<3 this PR!

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 7, 2020

Converted to draft as @babel/helper-compilation-targets should also be refactored.

@JLHwung JLHwung marked this pull request as draft September 7, 2020 19:25
@JLHwung JLHwung marked this pull request as ready for review September 8, 2020 15:23
import browserModulesData from "@babel/compat-data/native-modules";

import {
semverify,
semverMin,
isUnreleasedVersion,
getLowestUnreleased,
v,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're naming this v? I'm generally in favor of descriptive names, and I worry that its meaning will be obscured for future maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

+1 If validator is too verbose, we can still do import { validator as v }: at least you don't have to check into another file what v means.

@JLHwung JLHwung force-pushed the babel-helper-validator-option branch from aa94ca4 to 82a0546 Compare September 11, 2020 19:28
@JLHwung JLHwung merged commit f2da186 into babel:main Sep 24, 2020
@JLHwung JLHwung deleted the babel-helper-validator-option branch September 24, 2020 20:23
@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 Dec 25, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: preset-env PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants