Object type spread failing on imported type from other module or libdef (e.g. flow-typed)

[rosskevin]

babel-7 branch - I'm looking into this. found is in a local libdef.

import type {RoutingState} from 'found'

type Props = {
  ...$Exact<RoutingState>,
  classes: Object,
  children?: Element<*>,
  currentUser?: CurrentUser,
  t: Function,
  width: string,
}

TypeError: src/App/AppFrame.js: Cannot read property 'key' of undefined
    at /Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/makePropTypesAst.js:154:19
    at Array.map (native)
    at makeObjectAstForShape (/Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/makePropTypesAst.js:153:48)
    at makePropTypesAstForPropTypesAssignment (/Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/makePropTypesAst.js:43:12)
    at annotate (/Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/index.js:101:85)
    at /Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/index.js:181:20
    at Array.forEach (native)
    at PluginPass.ClassDeclaration (/Users/kross/projects/ui/node_modules/@alienfast/babel-plugin-flow-react-proptypes/lib/index.js:178:29)
    at newFn (/Users/kross/projects/ui/node_modules/babel-traverse/lib/visitors.js:266:21)
    at NodePath._call (/Users/kross/projects/ui/node_modules/babel-traverse/lib/path/context.js:71:18)

[rosskevin]

Problem identified: I expected the spreadShape I get from convertToPropTypes() to have properties that I then flatten and return. On an imported libdef type the spreadShape is raw, and doesn't have properties to flatten:

I'll need to get the actual definition instead of what appears to be a raw pointer.

[rosskevin]

@brigand I see this marked in code as a runtime merge, should this situation be treated like shape-intersect-runtime?

[brigand]

I think so, yes.

cc @mhaas (who wrote this code)

[rosskevin]

@mhaas - which test code is best for me to look at for this runtime intersection from a libdef?

[rosskevin]

This seems like a strange situation, as I need to alter the imported named type by obtaining the properties only (and in cases that don't use $Exact, modify the value isRequired for each). The same named type could be used without the spread as-is.

Where I do this manipulation makes sense (when not importing), perhaps someone can shed some light on why we are doing this at runtime (if it is really runtime) instead of transpilation time. If it is actually runtime, we won't have the libdef types from .js.flow and other local libdef files, so I'm a bit confused at this point, and wondering if these types shouldn't be resolved ahead of time?

That's the only way this could work, is it not?

[brigand]

This plugin doesn't read other files, so it doesn't know the types exported from other files until runtime. I'd like to keep it this way, but I'm convincible. @thejameskyle is working on a similar plugin that reads other files (not open source yet, afaik).

[brigand]

Ah, it is open source. https://github.com/thejameskyle/babel-plugin-react-flow-props-to-prop-types

[rosskevin]

Any es5 runtime will have none of the definitions found in a local libdef, meaning anything a user types themselves for an external library, or everything from the flow-typed project.

To me this means the usefulness is severely limited. In my case, I created a libdef for found because they have no intent on using flow type, but I do. I intended to refine it and submit as a PR to flow-typed, but that essentially makes it useless from a prop type, as everything will be resolved as any.

In my opinion, we must read these files at transpilation time to be useful.

I see the @thejameskyle plugin doesn't have issues enabled. Did that project split off due to the runtime concern? Is there a way to bring these two together for babel-7?

[rosskevin]

I saw the commentary on inlining types and solving the react-docgen problem, which is also a problem for me. I'm going to try out that new plugin and see where it leads.

[mhaas]

Can we transpile flow-typed?

[rosskevin]

@mhaas we should be able to interpret flow types from flow-typed (or other local libdefs) and create the equivalent prop-types, and this must be done at transpile-time.

[mhaas]

I agree that flow-typed is a great target. The libdefs have a different syntax, so that is going to be some work.

Why do we have to process flow-typed at transpile time?

[rosskevin]

flow-typed, which effectively is just another local libdef, is not available at runtime...that is if you are referring to runtime as a browser. runtime would need to be on that local filesystem, executed in that working dir to even have a chance to resolve them.

[mhaas]

@rosskevin We can transpile the local libdef just like we handle type exports. On the import side, the import type statement is transpiled into a regular import statement (or rather, a require call) to include the transpiled libdef - which is now just a regular ES module. Browserify/webpack should then ship that to the UA.

[mhaas]

@rosskevin Can you post your libdef for found? I am still trying to wrap my head around libdefs - I guess the only scenario we need to handle is something like this:

declare module "some-es-module" {
  // Declares a named "concatPath" export
  declare export type Foo = {a: string, b: number};
}

[mhaas]

I found some interesting discussion here:https://github.com/codemix/flow-runtime/issues/27

They apparently have a tool that preprocesses libdefs and makes them available.

[rosskevin]

@mhaas here is a found libdef.

It seems with object type spreads in particular, we really don't have a workaround for imported types, which is a huge problem for me now...

[mhaas]

@rosskevin Thanks!

It would seem to me that we need to do two things to handle import types from a libdef:

• Add support for parsing libdef files, which should be easy
• Integrate the libdef file with the file importing the type from the libdef, either by:
  • Using a module system, just exporting the types from a transpiled libdef and importing them - probably requires the user to add the flow-typed directory to the search path for module lookup in webpack/browserify
  • Including the relevant parts from the libdef file at transpile time

By the way, you mentioned that imported types do not work for object type spreads. This is only true for imported types from libdef files, correct?

[rosskevin]

Correct.

We also need to crawl/parse *.js.flow files from node_modules, these are untranspiled source files.

[rosskevin]

Just a quick note, I can see that https://github.com/thejameskyle/babel-plugin-react-flow-props-to-prop-types/blob/master/fixtures/imports/code.js is importing types from other files. This does not cover libdefs, but it is cross-module. Source seems to be _convertImportSpecifier. The resulting call to matchExported seems to be the resolution of the type.

I've found that plugin structure simpler and easier to follow, but it is far from complete and won't work with my projects. This plugin is much more complete.

Perhaps some inspiration from the other and a little refactoring here could get this plugin resolving local imports.

[rosskevin]

I've determined that importing from a libdef might be too ambitious near term, but importing cross-module from js source code seems plausible based on the code I found above. I can't dedicate the time at the moment, but if someone wants to give it a shot, I think the code pointers above will get it done. If I have time, I'll get to it.

[UchihaVeha]

babel-plugin-react-flow-props-to-prop-types - work with imports but dosn't work with object spread this module work with object spread but dosn't work with imports object spread type from other modules. Someone have solution?

import type { TodoView } from 'modules/todo/reducers';

type Props = {
  ...TodoView,
  onClick: (id: number) => void,
  onEdit: (id: number) => void
};

What to do to make it work?

[rosskevin]

@UchihaVeha - that is the exact case for this issue, it does not import the type from another file so it is null and fails. If type TodoView were in the same file, it would work.

[goodmind]

Any progress here?

[grrowl]

I believe this fails due to the same root cause:

export type QuerySingle<BaseQuery> = {|
  id: number,
  ...BaseQuery,
|}

in babel-plugin-flow-react-proptypes/lib/convertToPropTypes.js:165, spreadShape.properties.forEach explodes because spreadShape doesn't have a properties property at all (possibly because it's of type GenericTypeAnnotation?)

[shanez]

Is there any progress on this issue?

[TSMMark]

Any way I can help move this forward?

[brigand]

I apologize for such a long standing issue.

@TSMMark I might be able to work on this on the weekend, but if you have some time, I would very much appreciate a PR that either makes this fail softly or one that actually implements the expected behavior with exact types + spread.

[TSMMark]

@brigand I was trying to use this plugin but I couldn't seem to get it to add propTypes to any components for some reason. If I'm able to invest any time in it it's going to have to be trying to resolve that issue – sorry!

[TSMMark]

In my built files I'm seeing var _types = require("./types"); but then _types is never referenced after that. Any pointers?

[brigand]

@TSMMark that might happen if you have a type import (import type ...), since at the point the import is reached, we don't know whether or not we need it at runtime. The import is modified such that the babel flow plugin won't strip it, and then we can use it later if you refer to the type in a place that we process.