facebook/flow

`flow-remove-types`: `declare var` line stripped, leaving undefined object

rtrembecky opened this issue · 4 comments

rtrembecky commented

Flow version: 0.245.0

Expected behavior

Running flow-remove-types on these lines produces valid JS:

// node_modules/react-native/Libraries/Renderer/shims/ReactNativeTypes.js#129
// This validates that INativeMethods and NativeMethods stay in sync using Flow!
declare var ensureNativeMethodsAreSynced: NativeMethods;
(ensureNativeMethodsAreSynced: INativeMethods);

Actual behavior

This is the output I'm getting:

// This validates that INativeMethods and NativeMethods stay in sync using Flow!
                                                        
(ensureNativeMethodsAreSynced                );
  • Link to Try-Flow or Github repo: I don't think you have Try-Flow for flow-remove-types.

Additional context

In a React Native project, I'm trying to switch from babel-jest to @swc/jest. As it's React Native, some Flow-typed dependencies need to be transformed (transpiled) by Jest before running tests, which is a lengthy process, so swc could help reduce the time. However, swc doesn't have Flow support yet, so I'm using the https://github.com/changwoolab/flow-aware-swc-jest package, which simply uses flow-remove-types under the hood. This works for most of the Flow files, but crashes on node_modules/react-native/Libraries/Renderer/shims/ReactNativeTypes.js with:

  ● Test suite failed to run

    ReferenceError: ensureNativeMethodsAreSynced is not defined

I debugged the transpilation and noticed the above input/output - the line with declare var ensureNativeMethodsAreSynced is removed altogether, even though there's a usage of ensureNativeMethodsAreSynced on the next line. However, the next line is also just for the type check, so the type stripping should either:

  • leave the declaration intact
  • remove also the type-only usage
SamChou19815 commented

The behavior is not ideal, but as far as flow-remove-types is concerned, it's working as expected.

The operation of stripping types should literally just strip types, and also type only declarations. Whether the result of stripping types is still valid is left to people who write the code to deal with. In this case, flow-remove-types does exact that.

I'm not sure what extra magic the non-swc based solution is doing to cause this not to crash (transforming unknown globals access foo to something like window.foo?).

rtrembecky commented

I'm not sure as well, I didn't fully dive into babel-jest+@babel/plugin-transform-flow-strip-types (https://babeljs.io/docs/babel-plugin-transform-flow-strip-types), I just noticed they are probably using their own solution to get rid of Flow: https://github.com/babel/babel/blob/main/packages/babel-plugin-transform-flow-strip-types/src/index.ts

I understand that from a technical perspective, the flow-remove-types package should just remove types. However, from a user perspective (as is my case now), I expect to be able to use this official Flow-stripping package for any valid Flow-typed JS code (third-party RN package code here) to get valid vanilla JS. Such tooling should probably be a bit more robust.

Would it be possible to at least briefly compare https://github.com/facebook/flow/blob/main/packages/flow-remove-types/index.js with https://github.com/babel/babel/blob/main/packages/babel-plugin-transform-flow-strip-types/src/index.ts to see if there's a potential quick fix? And I can try to get the output from @babel/plugin-transform-flow-strip-types first to find out what it does with such code.

rtrembecky commented

thanks! very helpful. I might look into the file/imports resolution then when I have some time, I suspected Jest may be crawling and transforming some stuff unnecessarily... possibly also some configuration/behavior regarding barrel imports