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 transform-flow-comments for import types #9901
fix transform-flow-comments for import types #9901
Conversation
tanhauhau
commented
Apr 26, 2019
Q | A |
---|---|
Fixed Issues? | Fixes #6142 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
@@ -16,10 +16,12 @@ | |||
"@babel/plugin-syntax-flow": "^7.2.0" | |||
}, | |||
"peerDependencies": { | |||
"@babel/core": "^7.0.0-0" | |||
"@babel/core": "^7.0.0-0", | |||
"@babel/generator": "^7.0.0-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring users to install a new package is a breaking change.
I believe you can import it from @babel/core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interestingly @babel/core
does not export generate
function from @babel/generator
, should I export it out from @babel/core
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know that.
If we added a generate
export, we couldn't use it in this plugin anyway because it needs to be compatible with @babel/core@^7.0.0
. We can add @babel/generator
to the dependencies of this plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erm. then is it okay to use @babel/generator
as peerDependency
? since it's @babel/core
's dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because peer dependencies must be explicitly installed by the user, otherwise npm will log a warning
64870e6
to
e4dd3b4
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10766/ |
@existentialism is there something any improvements I can make for this PR to be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍