Strip Flow's new shorthand import-type specifiers #5035
Conversation
|
|
||
| ImportSpecifier(path) { | ||
| const { node } = path; | ||
| if (node.importKind !== null) { |
There was a problem hiding this comment.
I think it'd be safer to explicitly check the types we want to exclude. Sound good?
There was a problem hiding this comment.
Sure I can do that. Update coming right up!
The only kinds are "type", "typeof" and null. I don't expect we'll add a "value" since that's implied by null -- AKA leaving off any kind of keyword modifier.
There was a problem hiding this comment.
Yeah totally, just trying to be as future-proof as possible.
jeffmo
force-pushed
the
import_type_shorthand
branch
from
e05422b
to
12ffeb1
Compare
| } | ||
| }, | ||
|
|
||
| ImportSpecifier(path) { |
There was a problem hiding this comment.
I was looking into something unrelated today and came across something that may be a more general fix for this. How would you feel about moving this logic into
Then the Flow(path){ path.remove(); } above will automatically do this.
There was a problem hiding this comment.
I can do that.
FWIW: I wasn't able to actually find this virtual-types file while trying to find where import type was stripped (that file seems to be the magical file). I wonder if splitting these up like this is more confusing than it's worth?
(I'll defer to whichever you prefer)
There was a problem hiding this comment.
I think it'd be more consistent with what we do now, but yeah the current architecture is pretty tough to follow, no disagreement there. Definitely something we can fix.
jeffmo
force-pushed
the
import_type_shorthand
branch
from
12ffeb1
to
2b19b89
Compare
|
(assuming the code is acceptable): What's the procedure for pushing PRs like this that depend on other Babel-oriented packages getting published (i.e. babel/babylon#267)? |
|
I think the procedure right now is exactly what you are doing, it is a little bit tedious, but this is one of the drawbacks of moving babylon out of the monorepo. The test seems to fail. |
|
Hey @jeffmo! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time. OS: linux OS: linux OS: linux OS: linux |
| @@ -113,6 +113,8 @@ export let Flow = { | |||
| return node.importKind === "type" || node.importKind === "typeof"; | |||
| } else if (t.isExportDeclaration(node)) { | |||
| return node.exportKind === "type"; | |||
| } else if (t.isImportSpecifier(node)) { | |||
There was a problem hiding this comment.
This is failing because you'll need to add ImportSpecifier to the types: ["Flow", "ImportDeclaration", "ExportDeclaration"], list just above here.
There was a problem hiding this comment.
Oops, I assumed it was failing due to the parser -- didn't look closely. Thanks for the heads up!
jeffmo
force-pushed
the
import_type_shorthand
branch
from
2b19b89
to
6ba9345
Compare
|
@jeffmo The test passes now but we should also bump babel-traverse to depend strictly on babylon ^6.15.0 (rmacklin@883af44#diff-af2efa0aecdd6cb7790cd55ef11c04e6L15) |
jeffmo
force-pushed
the
import_type_shorthand
branch
from
6ba9345
to
b820d8e
Compare
|
Done! |
Current coverage is 89.20% (diff: 100%)@@ master #5035 diff @@
==========================================
Files 203 203
Lines 9817 9819 +2
Methods 1071 1071
Messages 0 0
Branches 2613 2615 +2
==========================================
+ Hits 8757 8759 +2
Misses 1060 1060
Partials 0 0
|
hzoo
added
the
PR: New Feature 🚀
lock
bot
added
the
outdated
This adds support to the
flow-strip-typestransform so that it strips type/typeof specifiers per the new syntax shorthand (details on the shorthand here).This depends on parsing support in Babylon, which is addressed by babel/babylon#267