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

improve node type definitions to avoid any's in generated types #11687

Merged
merged 4 commits into from Jun 8, 2020

Conversation

zxbodya
Copy link
Contributor

@zxbodya zxbodya commented Jun 6, 2020

Updates node type definitions in babel-types, to avoid any in generated types.

Q                       A
Fixed Issues? #11474
Patch: Bug Fix? yes(improve generate types)
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 6, 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 7470c76:

Sandbox Source
focused-perlman-jdd26 Configuration
sharp-cookies-6uog7 Configuration

@nicolo-ribaudo nicolo-ribaudo added area: typescript pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jun 6, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 6, 2020

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

value: {
validate: assertNodeType("StringLiteral"),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a breaking change, but this node is so new that I'd be fine with considering it a bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think generally such changes should be considered a bug fix because the validator should try its best to align with the parser behaviour.

@zxbodya
Copy link
Contributor Author

zxbodya commented Jun 8, 2020

rebased it on current master(resolving conflicts because of prettier update)

@nicolo-ribaudo
Copy link
Member

Thanks!

Comment on lines +282 to +285
comments: {
validate: assertEach(assertNodeType("Comment")),
optional: true,
},
Copy link

Choose a reason for hiding this comment

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

Just FYI this seems to have broken Jests toMatchInlineSnapshot matcher for tests written in TypeScript (jestjs/jest#10208)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, this is not intended, effect… probably this should be considered a breaking change and so to go to babel 8

@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 Sep 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Bug Fix 🐛 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