Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extracted CellMeasurer public interface #1058

Merged
merged 5 commits into from
Apr 7, 2018
Merged

Extracted CellMeasurer public interface #1058

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f2b8a7
Extracted CellMeasurer public interface
diogofcunha Mar 17, 2018
cf96312
Correct export/import for interface
diogofcunha Mar 17, 2018
086b76c
updated babel-plugin-flow-react-proptypes
diogofcunha Mar 17, 2018
bcbd20b
Added useESModules flag
diogofcunha Mar 18, 2018
6fa0b25
Removed extra space
diogofcunha Mar 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"jsnext:main": "dist/es/index.js",
"license": "MIT",
"scripts": {
"build:types": "flow-copy-source --ignore \"**/*.{jest,e2e,ssr,example}.js\" source/WindowScroller dist/es/WindowScroller && flow-copy-source --ignore \"**/*.{jest,e2e,ssr,example}.js\" source/AutoSizer dist/es/AutoSizer ",
"build:types": "flow-copy-source --ignore \"**/*.{jest,e2e,ssr,example}.js\" source/WindowScroller dist/es/WindowScroller && flow-copy-source --ignore \"**/*.{jest,e2e,ssr,example}.js\" source/AutoSizer dist/es/AutoSizer && flow-copy-source --ignore \"**/*.{jest,e2e,ssr,example}.js\" source/CellMeasurer dist/es/CellMeasurer",
"build": "npm run build:commonjs && npm run build:css && npm run build:es && npm run build:demo && npm run build:umd",
"build:commonjs": "npm run clean:commonjs && cross-env NODE_ENV=commonjs babel source --out-dir dist/commonjs",
"build:css": "postcss source/styles.css -o styles.css --use autoprefixer",
Expand Down
4 changes: 2 additions & 2 deletions source/CellMeasurer/CellMeasurer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Copy link
Owner

@bvaughn bvaughn Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.isRequired

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@TrySound TrySound Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I upgraded it some time ago and published 5 broken releases trying to fix its issues. We can try again.

Copy link
Contributor Author

@diogofcunha diogofcunha Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will probably just cherry pick your original commit and change the version to the latest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a few issues. I guess they are fixed now. I'm gonna land it after rollup integration.

Copy link
Contributor Author

@diogofcunha diogofcunha Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type Children = (params: {measure: () => void}) => React.Element<*>;

Expand All @@ -11,7 +11,7 @@ type Cell = {
};

type Props = {
cache: CellMeasurerCache,
cache: CellMeasureCache,
children: Children | React.Element<*>,
columnIndex?: number,
index?: number,
Expand Down
23 changes: 21 additions & 2 deletions source/CellMeasurer/CellMeasurerCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,24 @@ type IndexParam = {
index: number,
};

export interface CellMeasureCache {
hasFixedWidth(): boolean;
hasFixedHeight(): boolean;
has(rowIndex: number, columnIndex: number): boolean;
set(
rowIndex: number,
columnIndex: number,
width: number,
height: number,
): void;
getHeight(rowIndex: number, columnIndex?: number): number;
getWidth(rowIndex: number, columnIndex?: number): number;
}

/**
* Caches measurements for a given cell.
*/
export default class CellMeasurerCache {
export default class CellMeasurerCache implements CellMeasureCache {
_cellHeightCache: Cache = {};
_cellWidthCache: Cache = {};
_columnWidthCache: Cache = {};
Expand Down Expand Up @@ -174,7 +188,12 @@ export default class CellMeasurerCache {
: this._defaultHeight;
};

set(rowIndex: number, columnIndex: number, width: number, height: number) {
set(
rowIndex: number,
columnIndex: number,
width: number,
height: number,
): void {
const key = this._keyMapper(rowIndex, columnIndex);

if (columnIndex >= this._columnCount) {
Expand Down