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

Doesn't detect changes for functions that generate DOM component changes #154

Open
rob2d opened this issue Mar 27, 2017 · 13 comments
Open

Comments

@rob2d
Copy link

rob2d commented Mar 27, 2017

I have a component which has a class. I have noticed that reloading does not work if a function contained in a component has code which is changed. Example:

class MainApp extends Component
{
    constructor(props)
    {
        super(props);

        this.DOM =
        {
            G :     // generator functions
            {
                contactButton : (p)=>
                {
                    return (
                        <Button className={this.classes.contactButtonContainer}
                            onClick={()=> {location.href = p.url}}>
                            <i className={p.className}/>
                        </Button>
                    );
                }
            }
        };
    }
    render ()
    {
        return (
                <div className="appWrapper">
                    <div className="mainContent">
                        <AppHeader/>
                        <div>- insert body content here -</div>
                    </div>
                    <div className="appFooter">
                        {this.DOM.G.contactButton(
                        {
                            className : 'mdi mdi-github-box',
                            url       : 'http://somewhere'
                        })}
                        {this.DOM.G.contactButton(
                            {
                                className : 'rc3 rc3-npm-box',
                                url       : 'http://somewhere'
                            })}
                        {this.DOM.G.contactButton(
                            {
                                className : 'mdi mdi-code-tags',
                                url              : 'http://somewhere''
                            })}
                    </div>
                </div>
                );
        }
}

If I edit the definition of this.DOM.G.contactButton, no changes are detected when reloading. Also to note, I am using Redux & ReactRouter (@next/latest for each). I was very careful to set up each and require the proper middleware, but I'm not sure if this is an issue with that at this point.

@EugeneZ
Copy link
Contributor

EugeneZ commented Mar 27, 2017

This is fixed in livereactload@next. Another next to add to your list. 😄

See upgrade instructions here.

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

Amazing! Thanks for the quick reply :) Checking that out and will close right after.

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

Tried to follow the guide. Unfortunately, I am experiencing the following when trying to run my gulp setup now:

Unknown plugin "react-hot-loader/babel" specified in "base" at 3, attempted to resolve relative to "..."

[double checked and upgraded all babel related dependencies to the latest :/]

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

So I found the issue with that -- Just needed react-hot-loader@next!
Getting a familiar feeling here... 😄

Another thing now, it is still not refreshing/updating in the example case I provided, but I am also getting an invariant violation for including inject-tap-event-plugin more than once which was included as it continues to load the client app over and over again. I will try to alleviate that issue to see if it's what is causing it or whether it's livereactload at this point.

@EugeneZ
Copy link
Contributor

EugeneZ commented Mar 27, 2017

By default, the new livereactload assumes your code is idempotent (can be run over and over without issue) and the inject-tap-event-plugin is not idempotent. Wrap it in a try catch block.

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

thanks for the elegant solution.

[edit: nevermind :)]

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

To update: there are no more issues that might be conflicting with the build, but the issue posted above still seems to be there unfortunately 😐 It seems elements that aren't declared explicitly within an ES6 Component's render method are not reloaded/checked for changes. Is there a flag to always update when a bundle changes (and if not, is it possible)? The bundle change is detected, but the library simply decides there's no changes even though there are.

Right now the console is outputting this on these sorts of updates:

LiveReactload :: Reload complete!
build.js:348 LiveReactload :: Bundle changed
build.js:348 LiveReactload :: Nothing to reload

@milankinen
Copy link
Owner

To update: there are no more issues that might be conflicting with the build, but the issue posted above still seems to be there unfortunately

Do you mean the reload problem or inject-tap-event-plugin error?

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

Sorry for the ambiguity there -- the reload issue described/#154.

For now, at least functional Components don't have so much boilerplate for the specific example I brought up above. It might be something to keep an eye out for of since these things aren't generating reloads though.

@milankinen
Copy link
Owner

milankinen commented Mar 27, 2017

I think the cause of this issue might be that the factory function (DOM.G.contactButton) is bound in the component's constructor. Hence react-hot-loader can't intercept the function call and proxy the return value correctly.

How about if you extracted the contact button to its own separate stateless component? Would that work as expected? Something like this:

class MainApp extends Component
{
    constructor(props)
    {
        super(props);

        this.DOM =
        {
            G :     // generator functions
            {
                contactButton : (p) => 
                  <ContactButton {...p} btnClass={this.classes.contactButtonContainer} />
            }
        };
    }
    render ()
    {
        return (
                <div className="appWrapper">
                    <div className="mainContent">
                        <AppHeader/>
                        <div>- insert body content here -</div>
                    </div>
                    <div className="appFooter">
                        {this.DOM.G.contactButton(
                        {
                            className : 'mdi mdi-github-box',
                            url       : 'http://somewhere'
                        })}
                        {this.DOM.G.contactButton(
                            {
                                className : 'rc3 rc3-npm-box',
                                url       : 'http://somewhere'
                            })}
                        {this.DOM.G.contactButton(
                            {
                                className : 'mdi mdi-code-tags',
                                url              : 'http://somewhere'
                            })}
                    </div>
                </div>
                );
        }
}

function ContactButton({url, className, btnClass}) {
  return (
    <Button className={btnClass}
       onClick={()=> {location.href = url}}>
       <i className={className}/>
    </Button>
  );
}

@rob2d
Copy link
Author

rob2d commented Mar 27, 2017

Thanks for the thoughtful reply. I have actually resorted to doing just that. One reason why I had done things this way is material-ui now uses JSS and has to be compiled with a context binded to the style for classNames (I mean, I don't have to use JSS, but for the sake of consistency within these apps). I'm not sure if the technology within JSS that re-writes CSS to the DOM would have slowdown with more sheets generated per render to do that.

But I guess a little more boiler-plate here and there won't be the end of the world. Thanks 😄 👍

@EugeneZ
Copy link
Contributor

EugeneZ commented Apr 12, 2017

Apologies for the delay and for suggesting that the new beta would fix the issue. I got it confused with a different fix.

However, I recreated your code with react-hot-boilerplate here and it doesn't work even with the webpack non-LiveReactload approach.

Their issue for this is here.

They have a PR to fix this.

The advantage of the new 4.x LiveReactload is that you will get these fixes from react-hot-loader rather than from us.

@ThisIsRuddy
Copy link

Just got merged 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants