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.

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

setState doesn't trigger rerender #134

Merged
merged 11 commits into from
Jun 22, 2016
5 changes: 2 additions & 3 deletions Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,7 @@ Code examples in Markdown use the ES6+JSX syntax. They can access all the compon
You can also `require` other modules (e.g. mock data that you use in your unit tests) from examples in Markdown:

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

As an utility, also the [lodash](https://lodash.com/) library is available globally as `_`.
Expand All @@ -159,7 +158,7 @@ Each example has its own state that you can access at the `state` variable and
If you want to set the default state you can do:

```js
'key' in state || setState({key: 42});
setState({key: 42});
```

You *can* use `React.createClass` in your code examples, but if you need a more complex demo it’s often a good idea to define it in a separate JavaScript file instead and then just `require` it in Markdown.
Expand Down
6 changes: 3 additions & 3 deletions loaders/examples.loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ function examplesLoader(source, map) {
'}',
'module.exports = ' + JSON.stringify(examples).replace(
new RegExp(_.escapeRegExp(JSON.stringify(evalPlaceholder)), 'g'),
'function(code, setState) {' +
' var require = requireInRuntime;' +
' return eval(code);' +
'function(code) {' +
' var func = new Function (\"require\", \"state\", \"setState\", \"__initialStateCB\", code);' +
' return func.bind(null, requireInRuntime);' +
'}'
) + ';'
].join('\n');
Expand Down
67 changes: 56 additions & 11 deletions src/rsg-components/Preview/Preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ export default class Preview extends Component {
}
}

setComponentState(newState) {
this.componentState = {...this.componentState, ...newState};
setTimeout(this.executeCode.bind(this), 0);
}

compileCode(code) {
return transform(code, {
presets: ['es2015', 'react', 'stage-0']
Expand All @@ -57,15 +52,65 @@ export default class Preview extends Component {
}

try {
code = `
const state = Object.freeze(${JSON.stringify(this.componentState)});
${code}
let compiledCode = this.compileCode(this.props.code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

const?


// 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');
Copy link
Member

Choose a reason for hiding this comment

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

Will it work with ES6 classes and functional components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


// initiate state and set with the callback in the bottom component;
const initCode = `
var initialState = {};
${compiledCode.substring(0, splitIndex)}
__initialStateCB(initialState);
`;
// evalInContext returns a function which takes state, setState and a callback to handle the
// initial state and returns the evaluated code
const initial = this.props.evalInContext(initCode);

// 1) setup initialState so that we don't get an error;
// 2) use require data or make other setup for the example component;
// 3) return the example component
const renderCode = `
var initialState = {};
${compiledCode.substring(0, splitIndex)}
return ${compiledCode.substring(splitIndex)};
`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@mik01aj mik01aj Jun 9, 2016

Choose a reason for hiding this comment

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

Why not just add more arguments to evalInContext instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

let compiledCode = this.compileCode(code);
let component = this.props.evalInContext(compiledCode, this.setComponentState.bind(this));
const render = this.props.evalInContext(renderCode);

// wrap everything in a react component, such that we can leverage the state management of this component
class PreviewComponent extends React.Component {

constructor(props) {
super(props);

const state = {};
const initialStateCB = (initialState) => {
Object.assign(state, initialState);
};
const setStateError = (partialState) => {
const err = 'Calling setState to setup the initial state is deprecated. Use\ninitialState = ';
Object.assign(state, {error: err + JSON.stringify(partialState) + ';'});
};
initial({}, setStateError, initialStateCB);
this.state = state;
}

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

Choose a reason for hiding this comment

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

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

const state = this.state;
// pass through props from the wrapper component
return React.cloneElement(render(state, setState, null), this.props);
}
}

let wrappedComponent = (
<Wrapper>
{component}
<PreviewComponent />
</Wrapper>
);
ReactDOM.render(wrappedComponent, mountNode);
Expand Down