setState doesn't trigger rerender #134

saschatimme · 2016-04-26T07:01:30Z

This pull request should solve the issues in #51.

The previus method was basically, that we keep the state in memory and at every call of setState we render everything again. The problem is that this breaks animations as well as other "dom-dependent"actions.

This would have the following breaking API changes:

Instead of:

'count' in state || setState({ count : 1})

it would just be:

setState({ count : 1})

Another breaking change would be, that instead of:

const mockData = require('./mocks');
<Message content={mockData.hello}/>

you would have to call require directly in the component:

<Message content={require('./mocks').hello}/>

The reason is that I split the example code at the (single) react component. Everything before will be executed in the componentWillMount and the rest in the rendermethod of a react component (calling setState in a render method is not possible).

Would love to hear some feedback.

P.S.: The examples seem to be broken because of the "dog-names" packages, which runs into an error.

sapegin · 2016-04-26T07:26:50Z

src/rsg-components/Preview/Preview.js

+			// we split the setup of the state and the react component, since calling setState
+			// in render is not allowed.
+
+			const splitIndex = compiledCode.indexOf('React.createElement');


Will it work with ES6 classes and functional components?

In my examples does it work. The React.createElement () is just the result after the JSX transformation. So <Button /> becomes React.createElement(button,...)

sapegin · 2016-04-26T07:28:22Z

Thank you very much, this is very cool!

'count' in state || setState({ count : 1})

It was a hack so I won’t miss it ;-)

<Message content={require('./mocks').hello}/>

It’s fine when you need it only once (I think in most of the cases). Any way to reuse the required module?

mik01aj · 2016-04-26T08:23:07Z

I don't like this implicit separation of code into componentWillMount and render. And, to be honest, I don't have an idea how to make it explicit in a clean way. I just think that this feature will be confusing. Right now the semantic of a code example is simple: it executes like a regular function, and the last expression is the result, like in eval. When I define a variable and can't access it one line below, then something is broken.

sapegin · 2016-04-26T08:24:31Z

@mik01aj I agree but it fixes more things than breaks.

mik01aj · 2016-04-26T08:27:16Z

For me it will break many examples, I use mocks pretty often. For example:

var mocks = require('./mocks');
<div>
    <Avatar user={ mocks.exampleUser } />
    <Avatar user={ mocks.exampleUserWithoutAvatar } />
    <Avatar user={ mocks.exampleDeletedUser } />
    <Avatar user={ mocks.exampleUserWithInvalidAvatarUrl } />
</div>

sapegin · 2016-04-26T08:29:01Z

Maybe we could “hoist” setState calls — move only them to the componentWillMount? It’s more complicated and fragile but might work for more common use cases.

mik01aj · 2016-04-26T09:00:06Z

Let's think again.

We're talking about example usages of a component. If, in order to use a component, you need some special initialization in componentWillMount, then your component is not really reusable imho. getInitialState should be enough.

So maybe all we need is just some way to set the initial state, and then everything else could be done in render?

So I would propose this:

intialState = {x:1}
<button onClick={ () => setState({x: state.x + 1}) }>{ state.x }</button>

We could then run this once to get the initial state, and then just put this in render.

sapegin · 2016-04-26T09:01:41Z

Actually I like this idea very much ;-)

saschatimme · 2016-04-26T10:12:45Z

We should probably also consider the case that somebody requires a module and uses this to initiate the state and his component as well.

For this we need to make some assumptions / heuristics about the example codes.

I will give it a try in the next days :)

mik01aj · 2016-04-26T10:19:50Z

@saschatimme I think it will also work this way. That's what I think:

var DemoComponent = React.createClass({
    getInitialState: function () {
         var initialState = {};
         eval(EXAMPLE_CODE);
         return initialState;
    },
    render: function () {
         return eval(EXAMPLE_CODE);
    },
});

saschatimme · 2016-04-26T20:02:28Z

I got it running with the idea of @mik01aj, thanks!

I also added a error if the old syntax is used, since the setState would now cause a stack overflow.

Maybe you could could give this version a try with your examples and let me know if I overlooked something :)

sapegin · 2016-05-25T15:09:00Z

I need to try it one day.

tizmagik · 2016-06-08T16:30:58Z

Any update on this?

sapegin · 2016-06-08T16:33:28Z

@tizmagik Sorry didn’t have time to check it.

mik01aj · 2016-06-09T07:16:12Z

I tested it. It seems to fix the animations, but on the other hand, it breaks examples like this:

var ButtonToy = React.createClass({
    render: function () {
        return <Button>Click me!</Button>;
    },
});
<ButtonToy />

They fail with this error:

SyntaxError: Unexpected token )

I'm OK with this (I mean, I can update my examples so they don't create components), but this is a breaking change.

See also comments in the code. My biggest concern is this code splitting, it is a bad, bad idea. It will break examples like this:

let a = <div className="dot" />;
<Container>{ a }{ a }{ a }<div>{ a }{ a }</div></Container>

mik01aj · 2016-06-09T07:18:56Z

src/rsg-components/Preview/Preview.js

+					if (this.state.error) {
+						return <pre className={s.playgroundError}>{this.state.error}</pre>;
+					}
+					const setState = partialState => this.setState(partialState);


setState in React is defined as void setState(function|object nextState, [function callback]); you missed the 2nd argument.

sapegin · 2016-06-09T07:19:04Z

Then we can merge it and combine with hiding code examples by default in a single major release.

mik01aj · 2016-06-09T07:20:43Z

src/rsg-components/Preview/Preview.js

+			// the code contains the setup of the state and the react component to render.
+			// we split the setup of the state and the react component;
+
+			const splitIndex = compiledCode.indexOf('React.createElement');


This is a bad idea. And after the discussion in the PR, I thought we don't need any splitting anymore.

Yeah, I also thought that we’ve found a better solution.

saschatimme · 2016-06-09T09:07:28Z

src/rsg-components/Preview/Preview.js

+			const renderCode = `
+				var initialState = {};
+				${compiledCode.substring(0, splitIndex)}
+				return ${compiledCode.substring(splitIndex)};
 			`;


In order to pass the setState Function and the state into our component, we have to use new Function and cannot work with eval (this happens ins this.props.evalInContext). The current implementation uses the fact, that eval returns the result of the last statement, but for a with new Function generated function this is not the case. Hence to get a proper render Function with a return value, we have to insert somehow a return statement for the last component. This is the only reason splitIndex exists.

Why not just add more arguments to evalInContext instead?

what do you mean? I don't see what extra argument could solve the problem.

In examples.loader, there was this line:

'function(code, setState) {' +

The setState callback was passed to the component and it worked fine. If you needed to pass something more, you could just add arguments to this function. Why did you change it to use new Function()? Without that change, things would get simpler here.

sapegin · 2016-06-15T18:28:21Z

Would be very nice merge this PR. @saschatimme are you going to finish it?

saschatimme · 2016-06-15T19:56:10Z

Sorry I was quite busy the last two weeks but I will try to find a solution that doesn't involve any code splitting this weekend.

sapegin · 2016-06-15T19:57:20Z

@saschatimme Cooool!

saschatimme · 2016-06-18T10:49:15Z

@sapegin I removed the code splitting and the counter examples from mik01aj are now working

tizmagik · 2016-06-18T17:23:12Z

src/rsg-components/Preview/Preview.js

-			code = `
-				const state = Object.freeze(${JSON.stringify(this.componentState)});
-				${code}
+			let compiledCode = this.compileCode(this.props.code);


sapegin · 2016-06-22T06:15:17Z

@saschatimme Could you please update your branch and fix conflicts? I’ll merge it after that.

sapegin · 2016-06-22T06:35:20Z

There are a few ESLint warning now.

saschatimme · 2016-06-22T06:50:07Z

@sapegin sorry for that, it should be fixed now.

sapegin · 2016-06-22T08:16:18Z

Thanks!

saschatimme added 5 commits April 25, 2016 12:49

setState doesn't trigger rerender

6a4a20d

require at runtime & pass through props from custom wrapper

9152440

renaming

e314072

make eslint happy

3740832

update examples

24bb43e

sapegin reviewed Apr 26, 2016
View reviewed changes

initiate State with initialState = {..}

c403880

mik01aj reviewed Jun 9, 2016
View reviewed changes

sapegin mentioned this pull request Jun 9, 2016

Add code toggleability #150

Merged

saschatimme reviewed Jun 9, 2016
View reviewed changes

solution without codesplitting + fix setState signature

2af1677

tizmagik reviewed Jun 18, 2016
View reviewed changes

change unnecessary let to const

dbeda28

saschatimme and others added 2 commits June 22, 2016 08:24

resolve merge conflicts

67c442d

fix additional line in readme through merge

4860e51

fix lint errors

bb62976

sapegin merged commit 88de0c2 into styleguidist:master Jun 22, 2016

sapegin mentioned this pull request Jun 23, 2016

Changing state re-renders the component in a way that breaks animation #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setState doesn't trigger rerender #134

setState doesn't trigger rerender #134

saschatimme commented Apr 26, 2016

sapegin Apr 26, 2016

saschatimme Apr 26, 2016

sapegin commented Apr 26, 2016

mik01aj commented Apr 26, 2016 •

edited

Loading

sapegin commented Apr 26, 2016

mik01aj commented Apr 26, 2016

sapegin commented Apr 26, 2016 •

edited

Loading

mik01aj commented Apr 26, 2016

sapegin commented Apr 26, 2016

saschatimme commented Apr 26, 2016

mik01aj commented Apr 26, 2016

saschatimme commented Apr 26, 2016

sapegin commented May 25, 2016

tizmagik commented Jun 8, 2016

sapegin commented Jun 8, 2016

mik01aj commented Jun 9, 2016 •

edited

Loading

mik01aj Jun 9, 2016

sapegin commented Jun 9, 2016

mik01aj Jun 9, 2016

sapegin Jun 9, 2016

saschatimme Jun 9, 2016

mik01aj Jun 9, 2016 •

edited

Loading

saschatimme Jun 9, 2016

mik01aj Jun 9, 2016

sapegin commented Jun 15, 2016

saschatimme commented Jun 15, 2016

sapegin commented Jun 15, 2016

saschatimme commented Jun 18, 2016

tizmagik Jun 18, 2016

sapegin commented Jun 22, 2016

sapegin commented Jun 22, 2016

saschatimme commented Jun 22, 2016

sapegin commented Jun 22, 2016

setState doesn't trigger rerender #134

setState doesn't trigger rerender #134

Conversation

saschatimme commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin commented Apr 26, 2016

mik01aj commented Apr 26, 2016 • edited Loading

sapegin commented Apr 26, 2016

mik01aj commented Apr 26, 2016

sapegin commented Apr 26, 2016 • edited Loading

mik01aj commented Apr 26, 2016

sapegin commented Apr 26, 2016

saschatimme commented Apr 26, 2016

mik01aj commented Apr 26, 2016

saschatimme commented Apr 26, 2016

sapegin commented May 25, 2016

tizmagik commented Jun 8, 2016

sapegin commented Jun 8, 2016

mik01aj commented Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

sapegin commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik01aj Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin commented Jun 15, 2016

saschatimme commented Jun 15, 2016

sapegin commented Jun 15, 2016

saschatimme commented Jun 18, 2016

Choose a reason for hiding this comment

sapegin commented Jun 22, 2016

sapegin commented Jun 22, 2016

saschatimme commented Jun 22, 2016

sapegin commented Jun 22, 2016

mik01aj commented Apr 26, 2016 •

edited

Loading

sapegin commented Apr 26, 2016 •

edited

Loading

mik01aj commented Jun 9, 2016 •

edited

Loading

mik01aj Jun 9, 2016 •

edited

Loading