surveyjs / survey-library

Free JavaScript form builder library with integration for React, Angular, Vue, jQuery, and Knockout.
https://surveyjs.io/form-library
MIT License
4.09k stars 795 forks source link

onvaluechanged -> setState -> infinite loop #1308

Closed sekamaneka closed 5 years ago

sekamaneka commented 6 years ago

Are you requesting a feature, reporting a bug or asking a question?

Question

What is the current behavior?

state grows exponentially each time i choose an option in my survey.

What is the expected behavior?

setState is called once every time an option is updated without interfering with the survey.

Test code

    onValueChanged = (model) => (e) => {
        fetch('http://localhost:5000/api/post', {
            method: 'POST',
            headers: {
                'Accept': 'application/json',
                'Content-Type': 'application/json',
            },
            body: JSON.stringify(model.data)
        }).then(json => {
            this.setState({githubData: json});
            }
        );
    };

Specify your

I am trying to call my api everytime a user selects an option or changes it. With that information i am trying to update a sidebar. But if i call setstate in onvaluechanged first the state grows exponentially and second the options don't reset. So if normally i can choose one of three options, with setstate enabled i can choose all three. Any ideas?

dmitry-kurmanov commented 6 years ago

@swilso793 the code you provided is insufficient to repoduce the problem on our side. Could you please provide a minimal sample to reproduce it? You can use our quickstart Angular repo: https://github.com/surveyjs/surveyjs_angular_cli

sekamaneka commented 6 years ago

I'm sorry it's in React. I was coming of a 10 hour marathon without access to the docs and was a little bit tired. xD Here is the minimal code.

import React, {Component} from "react";
import * as Survey from "survey-react";
import "survey-react/survey.css";
import SurveyEditor from "./SurveyEditor";
import logo from "./logo.svg";
import "./App.css";
import "bootstrap/dist/css/bootstrap.css";
import StickyBox from "react-sticky-box";
import "./style.css";

import "jquery-ui/themes/base/all.css";
import "nouislider/distribute/nouislider.css";
import "select2/dist/css/select2.css";
import "bootstrap-slider/dist/css/bootstrap-slider.css";

import "jquery-bar-rating/dist/themes/css-stars.css";

import $ from "jquery";
import "jquery-ui/ui/widgets/datepicker.js";
import "select2/dist/js/select2.js";
import "jquery-bar-rating";

import * as widgets from "surveyjs-widgets";

widgets.icheck(Survey, $);
widgets.select2(Survey, $);
widgets.inputmask(Survey);
widgets.jquerybarrating(Survey, $);
widgets.jqueryuidatepicker(Survey, $);
widgets.nouislider(Survey);
widgets.select2tagbox(Survey, $);
widgets.signaturepad(Survey);
widgets.sortablejs(Survey);
widgets.ckeditor(Survey);
widgets.autocomplete(Survey, $);
widgets.bootstrapslider(Survey);

class App extends Component {
    json = {
        "pages": [
            {
                "name": "page1",
                "elements": [
                    {
                        "type": "radiogroup",
                        "name": "datastructure",
                        "title": "Which data structure/study design does your dataset have?",
                        "isRequired": true,
                        "choices": [
                            {
                                "value": "cross_sectional_study",
                                "text": "A cross-sectional study: (also known as a cross-sectional analysis, transverse study, prevalence study) is a type of observational study that analyzes data from a population, or a representative subset, at a specific point in time—that is, cross-sectional data. "
                            },
                            {
                                "value": "panel_data",
                                "text": "Panel data: In statistics and econometrics, panel data or longitudinal data[1][2] are multi-dimensional data involving measurements over time. Panel data contain observations of multiple phenomena obtained over multiple time periods for the same firms or individuals. (Paired Data) "
                            },
                            {
                                "value": "time_series",
                                "text": "Time series data: A time series is a series of data points indexed (or listed or graphed) in time order. (more than 100 measurments over time) "
                            }
                        ]
                    },
                ],
            }
        ]
    };
    onValueChanged = (model) => (e) => {
        this.setState({
            githubData: model.data
        })
    };

    constructor(props) {
        super(props);
        this.state = {
            requestFailed: false,
            githubData: {},
        };
    }

    componentWillMount() {
        import("icheck");
        //fetch('http://localhost:5000/')
        //this.getData();
        window["$"] = window["jQuery"] = $;
    }

    onComplete(result) {
        console.log("Complete! " + result);
    }

    render() {
        Survey.Survey.cssType = "bootstrap";
        var model = new Survey.Model(this.json);

        if (this.state.requestFailed) return <p>Failed!</p>
        if (!this.state.githubData) return <p>Loading...</p>
        return (
            <div className="App">
                <div className="App-header">
                    <img src={logo} className="App-logo" alt="logo"/>
                    <h2>Welcome</h2>
                </div>
                <div className="side-by-side">
                    <div className="content-body">
                        {/*If you want to show survey, uncomment the line below*/}
                        <h1>Welcome:</h1>
                        <Survey.Survey
                            model={model}
                            onComplete={this.onComplete}
                            onValueChanged={this.onValueChanged(model)}
                        />
                    </div>
                    <StickyBox className="content-sidebar" offset={10}>
                        {<h5>Click here.</h5>}
                        {<p>{this.state.githubData.datastructure}</p>}
                    </StickyBox>
                </div>
            </div>
        );
    }
}

export default App;
dmitry-kurmanov commented 6 years ago

We've reproduced the issue, thanks.

sekamaneka commented 6 years ago

Cool, let me know if i can assist.

xadie commented 6 years ago

It's not a React Issue. It's an issue in Survey.js-react. The cause of this is a combination of missuse of the componentWillReceiveProps event in reactSurvey.tsx in combination with the updateSurvey function in reactSurvey.tsx

protected updateSurvey(newProps: any) {
    if (newProps) {
      if (newProps.model) {
        this.survey = newProps.model;
      } else {
        if (newProps.json) {
          this.survey = new ReactSurveyModel(newProps.json);
        }
      }
    } else {
      this.survey = new ReactSurveyModel();
    }
    if (newProps) {
      for (var key in newProps) {
        if (key == "model" || key == "children") continue;
        if (key == "css") {
          this.survey.mergeCss(newProps.css, this.css);
          continue;
        }
        if (
          key.indexOf("on") == 0 &&
          this.survey[key] &&
          this.survey[key].add
        ) {
          let funcBody = newProps[key];
          let func = function(sender, options) {
            funcBody(sender, options);
          };
          this.survey[key].add(func); //<= will re-add already subscribed listeners.... 
        } else {
          this.survey[key] = newProps[key];
        }
      }
    }

Instead of replacing all listeners on an Event on each updateSurvey-call they are added.
Whenever the react event componentWillReceiveProps is triggered on the Survey Component it will add all by prop provided event listeners on top on the old list over the lifetime of the component. Basically the function bleeds old-state values into new-state values.

sekamaneka commented 6 years ago

That makes sense. I was expecting something like that. Very nice work!!

dmitry-kurmanov commented 6 years ago

@xadie yes you are right! @swilso793 We recommended to create model and add event listeners only once. For example:

  constructor(props) {
    super(props);
    var model = new Survey.Model(this.json);
    model.onComplete.add(this.onComplete);
    model.onValueChanged.add(this.onValueChanged);
   ...

    this.state = {
      ...
      surveyModel: model
    };
  }
  ...
  render() {
    ...
    return <Survey.Survey model={this.state.surveyModel} />;
  }

could you please try this approach?

andrewtelnov commented 6 years ago

@swilso793 @xadie I just changed the behavior. It is decribed in this issue.

Thank you, Andrew

sekamaneka commented 6 years ago

@dmitrykurmanov This works perfectly. Thanks guys :) This is a beautiful example of open source prowess.