Extracted CellMeasurer public interface #1058
Conversation
source/CellMeasurer/CellMeasurer.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| /** @flow */ | |||
| import * as React from 'react'; | |||
| import {findDOMNode} from 'react-dom'; | |||
| import CellMeasurerCache from './CellMeasurerCache.js'; | |||
| import {CellMeasureCache} from './CellMeasurerCache.js'; | |||
There was a problem hiding this comment.
This won't actually change the generated prop-types in any meaningful way:
// master
cache:
typeof _CellMeasurerCache2.default === "function"
? require("prop-types").instanceOf(_CellMeasurerCache2.default).isRequired
: require("prop-types").any.isRequired,
// this branch
cache:
typeof _CellMeasurerCache.CellMeasureCache === "function"
? require("prop-types").instanceOf(_CellMeasurerCache.CellMeasureCache)
.isRequired
: require("prop-types").any.isRequiredThere was a problem hiding this comment.
Made a few changes and the generated prop types now look like:
Will test it against react-virtualized-tree soon.
var babelPluginFlowReactPropTypes_proptype_CellMeasureCache = require('./types').babelPluginFlowReactPropTypes_proptype_CellMeasureCache || require('prop-types').any;
// ...
cache:
typeof babelPluginFlowReactPropTypes_proptype_CellMeasureCache === 'function'
? babelPluginFlowReactPropTypes_proptype_CellMeasureCache.isRequired
? babelPluginFlowReactPropTypes_proptype_CellMeasureCache.isRequired
: babelPluginFlowReactPropTypes_proptype_CellMeasureCache
: require('proptypes').shape(babelPluginFlowReactPropTypes_proptype_CellMeasureCache).isRequired
There was a problem hiding this comment.
@bvaughn our current version of babel-plugin-flow-react-proptypes is far behind (^13.0.0) when the package is on 21.0.0.
Interface support was added in 17.0.0 (brigand/babel-plugin-flow-react-proptypes#175), with the current version the propType for the interface is not being generated, causing the generated types.js to be empty.
Is there any know blocker in updating the package or can I go for that?
There was a problem hiding this comment.
I upgraded it some time ago and published 5 broken releases trying to fix its issues. We can try again.
There was a problem hiding this comment.
Thanks @TrySound, was checking the release history and can see we tried to update to 15.0.0 at the time, were our issues caused by this bug (brigand/babel-plugin-flow-react-proptypes@890c166) fixed in 16.0.0 release?
This issue (#991) opened at the time suggests it.
There was a problem hiding this comment.
will probably just cherry pick your original commit and change the version to the latest
There was a problem hiding this comment.
There was a few issues. I guess they are fixed now. I'm gonna land it after rollup integration.
There was a problem hiding this comment.
I have good news, tried to upgrade and all seems good on rv side, the build output seems to be converting flow types to propTypes correctly, this includes interfaces.
Will use a local npm link to test it against https://github.com/diogofcunha/react-virtualized-tree. If it works there I guess it's good to go.
There was a problem hiding this comment.
Tested this and after adding useESModules flag everything seems to be working fine for babel-plugin-flow-react-proptypes update, no tests were broken and I can't see any runtime errors.
Tomorrow will try to add a custom cache just to make sure the original issue gets resolved.
.babelrc.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| const env = process.env.NODE_ENV; | |||
| const env = process.env.NODE_ENV; | |||
Fixes #1026.
@bvaughn not totally confident about flow typings as I'm honestly not aware of best practices, shall we copy the types for the CellMeasurer as it's done for WindowScroller so that they can be pulled with the module and users can implement the interface or it's there anything I'm not aware of.