Recompose: [0.25.0 update] createEagerFactory production only error

[dlebedynskyi]

after update to 0.25.0 start getting

Error walking your react tree
TypeError: Class constructor OnMount cannot be invoked without 'new'
at createEagerElementUtil (/app/node_modules/recompose/utils/createEagerElementUtil.js:18:12)
at /app/node_modules/recompose/createEagerFactory.js:18:49
at ShouldUpdate.render (/app/node_modules/recompose/shouldUpdate.js:45:16)
at /app/node_modules/react-tree-walker/commonjs/index.js:148:35
at doTraverse (/app/node_modules/react-tree-walker/commonjs/index.js:78:21)

was all fine on 0.24.0

OnMount component is pretty plain

export default (timeout = 0) => BaseComponent =>
  class OnMount extends PureComponent {
    state = { isMounted: false };

    componentDidMount() {
      this.mounted = true;
      setTimeout(() => {
        if (this.mounted) {
          this.setState({ isMounted: true });
        }
      }, timeout);
    }

    componentWillUnmount() {
      this.mounted = false;
    }

    mouted = false;

    render = () =>
      <BaseComponent isMounted={this.state.isMounted} {...this.props} />;
  };

[istarkov]

Can you provide a full example on jsbin?
As I'm not see createEagerElementUtil in your code above.
Is it reproduced in DEV only mode or also in production?

cc @deepsweet

[dlebedynskyi]

@istarkov
only on production.
Dev runs fine.
And I was not using createEagerElementUtil specifically. just as part of compose chain
it seem that the problem is related to following change https://github.com/acdlite/recompose/pull/473
as close as I can make it https://jsfiddle.net/p3vsmrvo/4/

[istarkov]

That change haven't changed anything in production mode, seems like this https://github.com/acdlite/recompose/pull/463/files

[dlebedynskyi]

a bit more debugging.
I'm getting

type is  class OnMount extends _react.PureComponent {
// ..

from

var createEagerElementUtil = function createEagerElementUtil(hasKey, isReferentiallyTransparent, type, props, children) {
  if (!hasKey && isReferentiallyTransparent) {
    if (children) {
      return type(_extends({}, props, { children: children }));
    }
    console.log('type is ', type, props);
    return type(props);
  }

and if I try something like stuff this

const hoc = compose(
  pure,
  setPropTypes(propTypes),
  createEagerElement(onMount(), {}),

getting same error

TypeError: Class constructor OnMount cannot be invoked without 'new'
 at ../recompose/compose.js:22:18

[istarkov]

I just run that example in production mode and in development and see no error
Here is full example

import { compose } from 'recompose'
import React from 'react'
import ReactDOM from 'react-dom'

const onMount = (timeout = 0) => BaseComponent =>
  class OnMount extends React.PureComponent {
    state = { isMounted: false }

    componentDidMount() {
      this.mounted = true
      setTimeout(() => {
        if (this.mounted) {
          this.setState({ isMounted: true })
        }
      }, timeout)
    }

    componentWillUnmount() {
      this.mounted = false
    }

    mouted = false

    render = () =>
      <BaseComponent isMounted={this.state.isMounted} {...this.props} />
  }

const Counter = ({ count, increment, decrement }) =>
  <div>
    Count: {count}
    <div>
      <button onClick={increment}>+</button>
      <button onClick={decrement}>-</button>
    </div>
  </div>

const App = compose(onMount())(Counter)

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

I used create-react-app to build and run

[istarkov]

Your React version?

[istarkov]

Also in example you provided, again no eager factory is used

[istarkov]

But I'm able to reproduce it adding pure HOC in compose

[istarkov]

Minimal example to reproduce

import { compose, pure } from 'recompose'
import React from 'react'
import ReactDOM from 'react-dom'

const onMount = (timeout = 0) => BaseComponent =>
  class OnMount extends React.Component {
    render = () => <BaseComponent {...this.props} />
  }

const Counter = ({ count, increment, decrement }) => <div>HELLO</div>

const App = compose(pure, onMount())(Counter)

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

[istarkov]

To fix don't bind render method

const onMount = (timeout = 0) => BaseComponent =>
  class OnMount extends React.Component {
    render() {
      return <BaseComponent {...this.props} />
    }
  }

[istarkov]

I'll close this, feel free to reopen
If you are interesting about what the problem with bind and why it occurred now:
It is combination of 2 PRs #473 and #463
To check that component is a React Class Component we started to check the existence of render method in Component prototype in #463 and then in #473 we disabled that check used for optimisation purposes in DEV mode. So at production we get stuck.
I'm not sure do we need to provide an additional check that if type is not a react class it must be function. Imo yes it will be better to provide such check in devmode.
cc @wuct @deepsweet You thoughts?

[deepsweet]

I think that the concept mentioned in #473 is correct and we shouldn't rely on any "post-production" optimizations in any way. Something should be done on isClassComponent side, like "an additional check that if type is not a react class it must be function" you mentioned.

[dlebedynskyi]

@istarkov thanks for explaining and looking into this.
My main frustration is that it runs ok in development while crushes on production. This is not desired behavior and can lead to 'wtf' issues on production. I'd personally prefer not to have this optimization then have unexpected behavior.
2nd issue - was all fine in 0.14.
@deepsweet idea might be good option to look into

[wuct]

@istarkov I can't reproduce the problem using your minimal code. Here is the jsbin. Do I miss something?

[istarkov]

@wuct production only bug

[pk-nb]

Could we reopen this bug? We got burned by this as well. We use render = () => in lots of places it's breaking when applying `mapProps. It fails at the wrapped child's constructor.

Fails with [email protected] and [email protected]—works fine going back to 0.24.0

[istarkov]

Just stop bind render method. There is no sense in this at all.

[dlebedynskyi]

Just stop bind render method. There is no sense in this at all.

@istarkov it does really make much sense to prevent doing that. For purpose of recompose not to crush on production suddenly?
Even if we fix that there is no guaranty that someone won't do it again for whatever reason. Just to see breaking changes only production build.
Imo it is better without this change then having only production bugs.

[istarkov]

IMO its better without this optimizations too. Let us think a little what to do. May be we can provide additional checks at dev mode or something else cc @deepsweet

[istarkov]

Any help will be very appreciated.

[deepsweet]

I'd like to somehow extract this optimization from Recompose at all.

On Tue, Sep 26, 2017 at 16:59, Ivan Starkov [e-mail] wrote:

Reopened #489.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/acdlite/recompose","title":"acdlite/recompose","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/acdlite/recompose"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Reopened #489."}],"action":{"name":"View Issue","url":"https://github.com/acdlite/recompose/issues/489#event-1265932861"}}}

[wuct]

I agree it's better to leave this optimization to React.

[deepsweet]

so... let's remove it then? :)

[istarkov]

Ok

[istarkov]

Waiting for PR ;-)

[istarkov]

538 optimisations removed please test 0.26

[dlebedynskyi]

please test 0.26

Issue seem to be resolved. Thank you for the fix.