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

fix typescript for babel-types #10098

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Jun 14, 2019

Q                       A
Fixed Issues? Fixes #10065, Fixes #10075, Fixes #8375
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 14, 2019

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

@tanhauhau tanhauhau force-pushed the tanlh/fix/typescript-definitions-babel-types branch from 43d2917 to 5b83530 Compare June 14, 2019 15:08
@jhpratt
Copy link

jhpratt commented Jun 14, 2019

LGTM. I'd obviously prefer Babel used TS over Flow, as it's far more popular, but this PR should fix the issues I've found, at least.

Edit: Looking at CI, there's one instance where you put object instead of Object for the type declaration.

nicolo-ribaudo
nicolo-ribaudo previously approved these changes Jun 15, 2019
@tanhauhau
Copy link
Member Author

Looking at CI, there's one instance where you put object instead of Object for the type declaration.

Oops, because I copied over from generators/typescript 🙈

@tanhauhau tanhauhau force-pushed the tanlh/fix/typescript-definitions-babel-types branch from 4b39d4b to 6467cd1 Compare June 15, 2019 14:47
@existentialism existentialism added pkg: types area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jun 18, 2019
} else {
const functionName = toFunctionName(type);
lines.push(
`declare function _${functionName}(${args.join(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be export declare function ${type}? They always start with an uppercase letter (thus are valid identifiers), and it would match https://github.com/babel/babel/blob/6467cd1e27ffe2a3a1cfe97bc8c67b407cb1e7c4/packages/babel-types/src/builders/generated/index.js

Copy link
Member Author

Choose a reason for hiding this comment

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

no. this is for the edge case of super() and import() where you can't export declare function super(): SuperNode;

but instead I made it:

declare function _super(): SuperNode;
declare export { _super as super };

to workaround of not being able to export super and import

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo what do you think?

@nicolo-ribaudo
Copy link
Member

CircleCI failure is unrelated.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 5, 2019
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
5 participants