Fix Lint errors. #1321

Xiot · 2020-05-23T01:46:25Z

-turned off jsx-key errors in tests

fixed a bunch of other lint errors.

Commented some that were breaking due to floating point math.

chrisirhc

Seems fine, though the tagNames seem odd?

I see what you mean with moving to jest, might be easier to match those floating points.

chrisirhc · 2020-05-23T05:39:01Z

.gitignore

-showcase/bundle.js
-showcase/bundle.css
+bundle.js
+bundle.css


Are these exported bundles or bundles for the showcase? Should they be in. packages/showcase instead?

chrisirhc · 2020-05-23T05:39:52Z

packages/react-vis/src/animation.js

@@ -46,7 +46,7 @@ export function extractAnimatedPropValues(props) {
  const {animatedProps, ...otherProps} = props;

  return animatedProps.reduce((result, animatedPropName) => {
-    if (otherProps.hasOwnProperty(animatedPropName)) {
+    if (Object.prototype.hasOwnProperty.call(otherProps, animatedPropName)) {


Is that in case otherProps is not an object?

This was brought up by a line rule no-prototype-builtins to protect against the case that otherProps (or props itself) doesn't have its prototype set.
This can be done with Object.create(null)

chrisirhc · 2020-05-23T05:42:20Z

packages/react-vis/src/plot/series/arc-series.js

+              onMouseOver={e => this._valueMouseOverHandler(modifyRow(row), e)}
+              onMouseOut={e => this._valueMouseOutHandler(modifyRow(row), e)}
+              className={`${predefinedClassName}-path ${arcClassName} ${rowClassName}`}
+              d={arcedData(arcArg)}


I only fixed this one instance.
I did have to move the key property out of the object and explicitly set it in the jsx

chrisirhc · 2020-05-23T05:44:21Z

yarn.lock

@@ -7597,7 +7617,6 @@ string-width@^3.0.0, string-width@^3.1.0:
 string-width@^4.1.0:
  version "4.2.0"
  resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.0.tgz#952182c46cc7b2c313d1596e623992bd163b72b5"
-  integrity sha512-zUz5JD+tgqtuDjMhwIg5uFVV3dtqZ9yQJlZVfq4I01/K5Paj5UHj7VyrQOJvzawSVlKpObApbfD0Ed6yJc+1eg==


Are you perhaps using an old version of yarn?

It looks like volta only looks at the package.json in the current direction volta-cli/volta#378 (comment)
volta is pinned to 1.22.4 in the root package.json, but the package.json doesn't specify any volta config in any of the packages.

I'll copy the configs up

chrisirhc · 2020-05-23T05:45:07Z

packages/react-vis/tests/components/area-series-tests.js

@@ -32,12 +32,12 @@ test('AreaSeries: basic rendering', t => {
    'should find the right number of series'
  );
  t.equal(
-    $.find('.rv-xy-plot__series path').length,
+    $.find('path.rv-xy-plot__series').length,


This change seems odd. Did we change the render logic?

I didn't touch the render logic, but my guess is that it has to do with newer versions of enzyme (didn't look too deeply) and how it handles the css selectors.

The original $.find('.rv-xy-plot__series path').length was returning 0 elements.

Without the additional path selector we get this:

// $.find('.rv-xy-plot__series').debug() <path d="M0,250L125,0L250,166.66666666666669L250,183.33333333333334L125,150L0,233.33333333333334Z" className="rv-xy-plot__series rv-xy-plot__series--line area-chart-example" transform="translate(40,10)" onMouseOver={[Function]} onMouseOut={[Function]} onClick={[Function]} onContextMenu={[Function]} style={{...}} />

The root node is rendered as

<XYPlot width={300} height={300} className=""> <div style={{...}} className="rv-xy-plot"> <svg className="rv-xy-plot__inner" width={300} height={300} style={[undefined]} onClick={[Function]} onDoubleClick={[Function]} onMouseDown={[Function]} onMouseUp={[Function]} onMouseMove={[Function]} onMouseLeave={[Function]} onMouseEnter={[Function]} onTouchStart={[Function]} onTouchMove={[Function]} onTouchEnd={[Function]} onTouchCancel={[Function]} onWheel={[undefined]}> <AreaSeries className="area-chart-example" color="#12939a" data={{...}} stack={false} style={{...}} getNull={[Function: getNull]} marginLeft={40} marginTop={10} marginRight={10} marginBottom={40} innerHeight={250} innerWidth={250} animation={[undefined]} sameTypeTotal={1} sameTypeIndex={0} clusters={{...}} seriesIndex={0} _colorValue="#12939a" _opacityValue={1} xRange={{...}} yRange={{...}} colorRange={{...}} fillRange={{...}} strokeRange={{...}} opacityType="literal" sizeRange={{...}} getX={[Function]} getX0={[Function]} xDomain={{...}} getY={[Function]} getY0={[Function]} yDomain={{...}} getRadius={[Function]} getRadius0={[Function]} radiusDomain={{...}} getAngle={[Function]} getAngle0={[Function]} angleDomain={{...}} getColor={[Function]} getColor0={[Function]} colorDomain={{...}} getFill={[Function]} getFill0={[Function]} fillDomain={{...}} getStroke={[Function]} getStroke0={[Function]} strokeDomain={{...}} getOpacity={[Function]} getOpacity0={[Function]} opacityDomain={{...}} getSize={[Function]} getSize0={[Function]} sizeDomain={{...}} _allData={{...}} _adjustBy={{...}} _adjustWhat={{...}} _stackBy={[undefined]}> <path d="M0,250L125,0L250,166.66666666666669L250,183.33333333333334L125,150L0,233.33333333333334Z" className="rv-xy-plot__series rv-xy-plot__series--line area-chart-example" transform="translate(40,10)" onMouseOver={[Function]} onMouseOut={[Function]} onClick={[Function]} onContextMenu={[Function]} style={{...}} /> </AreaSeries> </svg> </div> </XYPlot>

Which makes sense given that the props that are passed to the area are

const AREA_PROPS = { className: 'area-chart-example', color: '#12939a', data: [ {x: 1, y: 5, y0: 6}, {x: 2, y: 20, y0: 11}, {x: 3, y: 10, y0: 9} ] };

The original version of the test was expecting that the rv-xy-plot__series className was provided directly to the AreaSeries, which it wasn't

I ran the tests on master and they all passed. (details below)

It looks like the rendering stayed the same, but the evaluation of the selector changed.
The new way (in the yarn-workspace branch) makes more sense.

The selector .rv-xy-plot__series path means find a path that is a child of an element with the rv-xy-plot__series class.
The correct selector for "find a path that has the class rv-xy-plot__series" is path.rv-xy-plot__series

These are the results on master.

// console.log($.find('.rv-xy-plot__series path').debug()) <path d="M0,250L125,0L250,166.66666666666669L250,183.33333333333331L125,150L0,233.33333333333334Z" className="rv-xy-plot__series rv-xy-plot__series--line area-chart-example" transform="translate(40,10)" onMouseOver={[Function]} onMouseOut={[Function]} onClick={[Function]} onContextMenu={[Function]} style={{...}} />

// console.log($.find('.rv-xy-plot__series').debug()) <path d="M0,250L125,0L250,166.66666666666669L250,183.33333333333331L125,150L0,233.33333333333334Z" className="rv-xy-plot__series rv-xy-plot__series--line area-chart-example" transform="translate(40,10)" onMouseOver={[Function]} onMouseOut={[Function]} onClick={[Function]} onContextMenu={[Function]} style={{...}} />

// console.log($.debug()) <XYPlot width={300} height={300} className=""> <div style={{...}} className="rv-xy-plot"> <svg className="rv-xy-plot__inner" width={300} height={300} style={[undefined]} onClick={[Function]} onDoubleClick={[Function]} onMouseDown={[Function]} onMouseUp={[Function]} onMouseMove={[Function]} onMouseLeave={[Function]} onMouseEnter={[Function]} onTouchStart={[Function]} onTouchMove={[Function]} onTouchEnd={[Function]} onTouchCancel={[Function]} onWheel={[undefined]}> <AreaSeries className="area-chart-example" color="#12939a" data={{...}} stack={false} style={{...}} getNull={[Function]} marginLeft={40} marginTop={10} marginRight={10} marginBottom={40} innerHeight={250} innerWidth={250} animation={[undefined]} sameTypeTotal={1} sameTypeIndex={0} clusters={{...}} seriesIndex={0} _colorValue="#12939a" _opacityValue={1} xRange={{...}} yRange={{...}} colorRange={{...}} fillRange={{...}} strokeRange={{...}} opacityType="literal" sizeRange={{...}} getX={[Function]} getX0={[Function]} xDomain={{...}} getY={[Function]} getY0={[Function]} yDomain={{...}} getRadius={[Function]} getRadius0={[Function]} radiusDomain={{...}} getAngle={[Function]} getAngle0={[Function]} angleDomain={{...}} getColor={[Function]} getColor0={[Function]} colorDomain={{...}} getFill={[Function]} getFill0={[Function]} fillDomain={{...}} getStroke={[Function]} getStroke0={[Function]} strokeDomain={{...}} getOpacity={[Function]} getOpacity0={[Function]} opacityDomain={{...}} getSize={[Function]} getSize0={[Function]} sizeDomain={{...}} _allData={{...}} _adjustBy={{...}} _adjustWhat={{...}} _stackBy={[undefined]}> <path d="M0,250L125,0L250,166.66666666666669L250,183.33333333333331L125,150L0,233.33333333333334Z" className="rv-xy-plot__series rv-xy-plot__series--line area-chart-example" transform="translate(40,10)" onMouseOver={[Function]} onMouseOut={[Function]} onClick={[Function]} onContextMenu={[Function]} style={{...}} /> </AreaSeries> </svg> </div> </XYPlot>

chrisirhc · 2020-05-23T05:46:12Z

packages/react-vis/tests/components/bar-series-tests.js

@@ -28,14 +28,14 @@ test('BarSeries: Showcase Example - BarChart', t => {
    'should find the right number of bars'
  );
  t.equal(
-    $.find('.vertical-bar-series-example').length,
+    $.find('g.vertical-bar-series-example').length,


Is the g necessary for tests to pass?

yes. The series component is passed the className as well, so the original find matched all of the other components as well.

console.log($.find('.vertical-bar-series-example').map(x => x.name())) [ 'VerticalBarSeries', 'BarSeries', 'g' ]

chrisirhc · 2020-05-23T05:48:16Z

packages/react-vis/tests/components/parallel-coordinates-tests.js

@@ -74,15 +80,16 @@ test('Parallel Coordinates: Animated Parallel Coordinates ', t => {
    'should find the right number of axes'
  );
  t.equal(
-    $.find('.rv-parallel-coordinates-chart-line').length,
+    $.find('path.rv-parallel-coordinates-chart-line').length,


Is there a lint rule catching these? Otherwise, might be better to leave it as before since the element/tagname is an implementation detail.

These are causing the tests to fail.
If we want to better hide the implementation details, then we should probably switch to shallow rendering.
I've never been a fan of mount in tests as it causes way more rendering and logic to be executed than I generally want to test.

chrisirhc · 2020-05-23T05:48:52Z

packages/react-vis/tests/components/custom-svg-series-tests.js

@@ -43,7 +43,7 @@ test('CustomSVGSeries: Showcase Example - CustomSVGRootLevelComponent', t => {
  const $ = mount(<CustomSVGRootLevelComponent />);
  t.equal(
    $.text(),
-    '1.01.52.02.53.068101214x: 0y: 125x: 87.5y: 75x: 125y: 250x: 250y: 0x: 187.5y: 200',
+    '1.01.52.02.53.068101214x: 0y: 125x: 87.5y: 75.00000000000001x: 125y: 250x: 250y: 0x: 187.5y: 200',


Interesting, did computation logic change?

No logic changed, but directly comparing floats in JS is rarely a good idea as it tends to do stuff like this.
There were a few tests that I commented out because it was doing a similar thing.

chrisirhc · 2020-05-23T05:50:25Z

packages/react-vis/tests/utils/scales-utils-tests.js

@@ -637,7 +637,7 @@ test('scales-utils #_getScaleDistanceAndAdjustedDomain', t => {
    FAKE_DATA,
    logScaleObject
  );
-  const expectedLogResults = {distance: Infinity, domain0: 0.1, domainN: 1.5};
+  const expectedLogResults = {distance: NaN, domain0: 0.1, domainN: 1.5};


Ya... I didn't put too much thought into this one.

reitlepax · 2020-05-23T14:00:15Z

packages/react-vis/src/animation.js

@@ -59,6 +59,7 @@ class Animation extends PureComponent {
    this._updateInterpolator(props);
  }

+  // eslint-disable-next-line react/no-deprecated
  componentWillUpdate(props) {


What bout renaming it to UNSAFE_ componentWillUpdate instead ?

That way we don't need an eslint-disable and we are being transparent about the fact that we use unsafe methods that need to be fixed easily

same comment for all the // eslint-disable-next-line react/no-deprecated that were added

It depends on what the minimum version of react that we want to support, and what version the UNSAFE_ prefix was introduced.
Since we want to use hooks the lowest we can go is 16.8 but I haven't looked to see if there is anything else that we'd like to have that is in a version later than that.

Indeed you're right! Let's ignore the lint error for now then, good call

It looks like the UNSAFE_ prefixes were added in [email protected] so we should be safe to move over to the UNSAFE_ variants.

Xiot · 2020-05-23T14:28:17Z

Seems fine, though the tagNames seem odd?

I see what you mean with moving to jest, might be easier to match those floating points.

Jest wont really help us if we try to render floats to strings and compare it with .text()
It also won't help if we try to compare complex objects that have floats in them.

Jest will help us with being able to run subsets of tests. For this I had to comment out all but one test file at a time in order to run them with some hope of fixing these tests.

Chris Thomas added 6 commits May 22, 2020 21:44

Fix Lint errors.

8e00a52

remove unpm

9ab9b4b

Fixed all of the unit tests.

79d792f

Commented some that were breaking due to floating point math.

remove unpm

f98af87

fix lint

c8c651f

fix lint for real

c8cfeab

chrisirhc approved these changes May 23, 2020

View reviewed changes

reitlepax reviewed May 23, 2020

View reviewed changes

Xiot merged commit eeb4962 into yarn-workspaces May 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Lint errors. #1321

Fix Lint errors. #1321

Xiot commented May 23, 2020

chrisirhc left a comment

chrisirhc May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

chrisirhc May 23, 2020

Xiot May 23, 2020

reitlepax May 23, 2020

reitlepax May 23, 2020

Xiot May 23, 2020

reitlepax May 23, 2020

Xiot May 23, 2020

Xiot commented May 23, 2020

Fix Lint errors. #1321

Fix Lint errors. #1321

Conversation

Xiot commented May 23, 2020

chrisirhc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiot commented May 23, 2020