search

FormidableLabs / victory

A collection of composable React components for building interactive data visualizations
http://commerce.nearform.com/open-source/victory/
Other
11.02k stars 524 forks source link

[BUG] Undefined context variable in Victory mixin static methods. #778

Closed richardbutler closed 6 years ago

richardbutler commented 7 years ago

I get the following exception when running my cursor over a chart that uses any mouse container:

Uncaught TypeError: Cannot read property 'mouseMoveMutationId' of undefined
    at onMouseMove (victory-selection-container.js?0c76:88)
    at Object.combinedHandler [as onMouseMove] (create-container.js?147b:50)
    at onEvent (events.js?bfaa:177)
    at wrapper (_createHybrid.js?887c:87)
    at Object.ReactErrorUtils.invokeGuardedCallback (ReactErrorUtils.js?dc41:69)
    at executeDispatch (EventPluginUtils.js?5d8c:85)
    at Object.executeDispatchesInOrder (EventPluginUtils.js?5d8c:108)
    at executeDispatchesAndRelease (EventPluginHub.js?0f32:43)
    at executeDispatchesAndReleaseTopLevel (EventPluginHub.js?0f32:54)
    at Array.forEach (<anonymous>)
    at forEachAccumulated (forEachAccumulated.js?e2c3:24)
    at Object.processEventQueue (EventPluginHub.js?0f32:254)
    at runEventQueueInBatch (ReactEventEmitterMixin.js?91f8:17)
    at Object.handleTopLevel [as _handleTopLevel] (ReactEventEmitterMixin.js?91f8:27)
    at handleTopLevelImpl (ReactEventListener.js?944f:72)
    at ReactDefaultBatchingStrategyTransaction.perform (Transaction.js?f15f:143)
    at Object.batchedUpdates (ReactDefaultBatchingStrategy.js?e9be:62)
    at Object.batchedUpdates (ReactUpdates.js?8e6b:97)
    at dispatchEvent (ReactEventListener.js?944f:147)

It would seem that a this reference in one of the handlers is erroneously pointing to the top-level scope of its module as a Babelification artifact. i.e.:

// victory-selection-container:1
var _this2 = this;

// victory-selection-container:88
if (mutations.id !== _this2.mouseMoveMutationId) {
  // eslint-disable-line
  _this2.mouseMoveMutationId = mutations.id; // eslint-disable-line
  return mutations.mutations;
}

What is this intended to point at? The ES6 source is an arrow function which looks like it will likely not have a this scope.

Here's a simplified version of my code:

import React, { Component } from 'react';
import { render } from 'react-dom';
import { VictoryChart, VictoryLine, createContainer } from 'victory';

const HybridContainer = createContainer('cursor', 'selection');
const stubData = [{
  time: new Date(2017, 7, 10, 11),
  midPx: 100
}, {
  time: new Date(2017, 7, 10, 12),
  midPx: 101
}, {
  time: new Date(2017, 7, 10, 13),
  midPx: 102
}, {
  time: new Date(2017, 7, 10, 14),
  midPx: 103
}, {
  time: new Date(2017, 7, 10, 15),
  midPx: 104
}];

function App () {
  const container = (
    <HybridContainer dimension="x" />
  );

  return (
    <VictoryChart containerComponent={ container }>
      <VictoryLine
        x="time"
        y="midPx"
        data={ stubData }
        interpolation="stepAfter"
        style={{
          data: {
            fill: 'transparent',
            stroke: '#999'
          }
        }}
      />
    </VictoryChart>
  );
}

render(<App />, document.getElementById('root'));

Any ideas?

boygirl commented 7 years ago

@richardbutler I'm not able to reproduce your issue: https://jsfiddle.net/boygirl/ot7L4jLw/

please note that some naming changes have occurred for container props in the latest version. A full list of breaking naming changes can be found in the changelog.

What version of Victory are you using?

andyshora commented 6 years ago

@richardbutler I see the same issue. Did you ever get this one resolved?

andyshora commented 6 years ago

@richardbutler your diagnosis is correct. Once this module makes it way your transpilation, it the references get mangled.

A simple solution which worked for us, is to exclude this module (and potentially all others) from your babel loader:

loader: 'babel-loader',
exclude: /node_modules/
richardbutler commented 6 years ago

@andyshora I gave up on this ended up writing a mouse container of my own as I had specific requirements.

I'm puzzled by that fix though because I was under the illusion that babel-loader ignores /node_modules/ by default.

ryan-roemer commented 6 years ago

I don't believe that there is a default exclude for babel-loader --> https://github.com/babel/babel-loader#babel-loader-is-slow

Additionally, as a helpful tip -- it's a best practice to use include rather than exclude to really hone down exactly your source that actually needs the babelling. (h/t sokra)

@richardbutler -- Can you either close this issue if you're no longer hitting it b/c of alternate path or help us with a reproducible error to diagnose (preferably as a minimal repository with install + error reproduction steps)? Thanks!

boygirl commented 6 years ago

I'm going to close this issue, but I will reopen it if more information / reproduction is provided.

ivansugi commented 6 years ago

hi all,

i have the same problem, i use "victory": "0.24.5", "react": "15.6.2","prop-types": "15.6.0", as

loader: 'babel-loader',
exclude: /node_modules/,
query: {
     presets: ['es2015', 'react', 'stage-0'] 
}

var _this2 = this; is undefined.. can't figure out what the value of this on victory-cursor-container.js, 

import React, { Component } from 'react'
import { connect } from 'react-redux'
import { Link } from 'react-router'
import { Alert, Row, Col, Table, Checkbox, FormControl, ControlLabel, Button, InputGroup } from 'react-bootstrap'
import CurrencyInput from 'react-currency-input'
import { toggleLoginModal, toggleSideNav } from '../../modules/layout'

import BoxRecommendedArticles from '../articles/BoxRecommendedArticles'

import { Box, BoxHeader, BoxBody, BoxFooter } from '../common/Box'

import Slider, { createSliderWithTooltip } from 'rc-slider'
import { monthsShort } from 'moment';

import { Tab, Tabs, TabList, TabPanel } from 'react-tabs'
import {VictoryChart, VictoryLine, VictoryCursorContainer} from 'victory';
...
    dataRent = () => {
        var data = [];
        var baseline = this.state.rent;
        for (var i = 1; i <= 25; i++) {
            baseline = baseline+ (baseline *5/100);
            data[i] = {
                a:i,
                b:baseline
            }
        }
       // debugger;
        return data;
    }
    dataMortgage = () => {
        var data = [];
        var baseline = this.state.monthly;
        for (var i = 1; i <= 25; i++) {
            baseline = baseline+ (baseline *1/100);
            data[i] = {
                a:i,
                b:baseline
            }
        }
        //debugger;
        return data;
    }
...
render() {
        //const CustomContainer = createContainer("cursor", "selection");
        return (
              <div>
                    <VictoryChart
                        domainPadding={{ x: 20 }}
                        animate={{duration: 500}}
                        containerComponent={
                            <VictoryCursorContainer/>
                          }
                    >
                       <VictoryLine
                        style={{
                            data: { stroke: "tomato" }
                        }}
                        data={this.dataRent()}
                        x="a"
                        y="b"
                        />
                        <VictoryLine
                        style={{
                            data: { stroke: "blue" }
                        }}
                        data={this.dataMortgage()}
                        x="a"
                        y="b"
                        />

                    </VictoryChart>
               </div>
       )
}

i hope that's help pin pointing the issue

ryan-roemer commented 6 years ago

@ivansugi -- Can you create a minimal public repository with code + install steps + instructions to reproduce the error? If we can reproduce the error then we'll reopen the issue. Thanks!

ivansugi commented 6 years ago

@ryan-roemer ,

this a commercial project, will link to the dev site helps? if not I will try to create new project to demonstrate this problem.

thanks!

ryan-roemer commented 6 years ago

@ivansugi -- You can link the site, but I don't think we'll get what we need. What I'm asking for is a demo site that you create that has just the minimum amount of stuff to reproduce the error -- so we can inspect, tweak, build, etc. things and figure out what's up. And we need to see how your build is configure to pinpoint exactly what you're bringing in from victory and what transforms are happening.

This also helps us more clearly identify if the issue is indeed a victory problem once all the rest of your proprietary app is gone.

On a completely separate note, does this error occur when unminified? If only occurs when minified, check your version of uglify-es that's being used and try upgrading that manually in your project. What you're really seeing here is either a babel transpile and/or a minification error and very unlikely to be an actual victory issue.

ivansugi commented 6 years ago

@ryan-roemer,

yes sure will separate codebase that will only display this problem let see how it goes...

thanks for your help

ivansugi commented 6 years ago

@ryan-roemer , i make a repo for this one, i wrote the readme.md as well. i hope this help.

https://github.com/ivansugi/victory-bug

thanks.

ryan-roemer commented 6 years ago

Thanks @ivansugi -- I can reproduce the exception with your project. Looking into this more, we have some weird binding for this that I'll detail more later today. Reopening for now.

@boygirl -- If you're around the office today, let's catch up!

ryan-roemer commented 6 years ago

@boygirl -- So here's the pattern that looks weird: https://github.com/FormidableLabs/victory-chart/blob/master/src/components/containers/victory-cursor-container.js#L49-L68 . The uses of this don't seem right given this is a static member which means this is scoped to file level.

Here's what it breaks down to in a simplified example:

import React from 'react';

export class Button extends React.Component {
  static foo = [{
    bar: () => {
      return this.member;
    }
  }];
}

transpiles to (abbreviated)

// ... SNIPPED ...
var _this2 = this;
var _react = require('react');

// ... SNIPPED ...
var Button = exports.Button = function (_React$Component) {
  // ... SNIPPED ...
}(_react2.default.Component);

Button.foo = [{
  bar: function () {
    return _this2.member;
  }
}];

But, when bundling in webpack, this can end up as:

var _this2 = undefined;

And that's why the code is breaking. So there's two levels of things going on:

  1. Is this a webpack bug to not add file-scope this?
  2. In either case, what should we be binding to.

So it looks like we're binding that mouse id and other stuff to file / global level?

Perhaps tellingly, when I remove the global eslint-disable-lines in those functions around the this calls, we get this lint error:

/Users/rye/scm/fmd/victory-chart/src/components/containers/victory-cursor-container.js
  52:30  error  Unexpected 'this'  no-invalid-this
  53:11  error  Unexpected 'this'  no-invalid-this

which actually seems appropriate.

So, I think my questions are:

Whatever we decide, we should investigate and scrub across all of the victory projects for this pattern...

ryan-roemer commented 6 years ago

I did a grep on that top level context variable being created by babel transpilation in all victory repos and here's what I found:

$ lank exec -- 'egrep -rn "^var .* = this;" lib || echo "NOT FOUND"'
[victory:exec:start]         egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29127)
[victory-core:exec:start]    egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29129)
[victory-chart:exec:start]   egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29130)
[victory-pie:exec:start]     egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29132)
[victory-native:exec:start]  egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29134)
[victory:stdout]             NOT FOUND
[victory:stdout]             
[victory:exec:finish]        egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29127)
[victory-native:stdout]      NOT FOUND
[victory-native:stdout]      
[victory-native:exec:finish] egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29134)
[victory-core:stdout]        NOT FOUND
[victory-core:stdout]        
[victory-core:exec:finish]   egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29129)
[victory-pie:stdout]         NOT FOUND
[victory-pie:stdout]         
[victory-pie:exec:finish]    egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29132)
[victory-chart:stdout]       lib/components/containers/victory-brush-container.js:18:var _this2 = this;
[victory-chart:stdout]       lib/components/containers/victory-cursor-container.js:22:var _this2 = this;
[victory-chart:stdout]       lib/components/containers/victory-selection-container.js:6:var _this2 = this;
[victory-chart:stdout]       lib/components/containers/victory-voronoi-container.js:14:var _this2 = this;
[victory-chart:stdout]       lib/components/containers/victory-zoom-container.js:14:var _this3 = this;
[victory-chart:stdout]       
[victory-chart:exec:finish]  egrep -rn "^var .* = this;" lib || echo "NOT FOUND" (pid: 29130)
[lank:main]                  Done.

and it looks like our world of things to potentially refactor is all in victory-chart: