Detect and apply stripping flow types #1864
Conversation
|
This looks great. Excited to see it implemented. 🗡 |
src/transforms/babel.js
Outdated
| if (!isSourceModule && asset.contents.includes('@flow')) { | ||
| if ( | ||
| !isSourceModule && | ||
| asset.contents.substring(0, 20).indexOf('@flow') > -1 |
There was a problem hiding this comment.
https://jsperf.com/includes-vs-substring-includes-vs-indexof/1 Performance test for this line
src/transforms/babel.js
Outdated
| */ | ||
| function getFlowConfig(asset, isSourceModule) { | ||
| if ( | ||
| !isSourceModule && |
There was a problem hiding this comment.
Should this be the inverse? Wouldn't we want to enable flow on app code and locally linked modules? I'm not sure enabling it on published node_modules is a good idea. WDYT?
There was a problem hiding this comment.
The issue is published code, as they shouldn't ship with flow types, local code should follow babelrc.
Shipped code should probably use flow comments or seperate .flow files
There was a problem hiding this comment.
I think it would be a nice feature to enable flow automatically on source code just like we do for JSX.
Shipped code should generally be pre-transpiled to remove the types I thought... Not sure, I guess it doesn't hurt to automatically do it for everything.
There was a problem hiding this comment.
It should, but apparently not every module does it as you can see in the linked issue
src/transforms/babel.js
Outdated
| function getFlowConfig(asset, isSourceModule) { | ||
| if ( | ||
| !isSourceModule && | ||
| asset.contents.substring(0, 20).indexOf('@flow') > -1 |
There was a problem hiding this comment.
Should we ensure that this is inside a comment? like look for // @flow or something?
There was a problem hiding this comment.
The issue is that it can be // @flow or /* @flow */ or any other variation of comment. So either we'd have to use a regex or do a babel walk to just detect this. Which would probably slow things down.
I can change it to a regex or babel walk just not sure about performance (Probably regex).
There was a problem hiding this comment.
Yeah a regex would be ok (on the substring). Should be fast enough.
| @@ -148,6 +149,11 @@ async function getBabelConfig(asset) { | |||
| return jsxConfig; | |||
| } | |||
|
|
|||
| // If there is a Flow config, return that | |||
| if (flowConfig) { | |||
There was a problem hiding this comment.
I guess we also need to merge the flow config with the generated env/jsx configs above right?
Closes #1863