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

Check if WrappedComponent implementation changed since last render to fix HMR #505

Merged
merged 1 commit into from Mar 6, 2017

Conversation

Projects
None yet
4 participants
@brunoabreu
Copy link
Contributor

brunoabreu commented Mar 3, 2017

As documented in:
#174
apollographql/apollo-client#764
https://github.com/HriBB/react-apollo-rhl-problem

Hot Module Replacement doesn't work when your component is wrapped by the GraphQL HOC. This happens because render() reutilizes this.renderedElement even though the WrappedComponent implementation changed during hot reloading.

  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Mar 3, 2017

@brunoabreu: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -577,7 +577,7 @@ export default function graphql(
const clientProps = this.calculateResultProps(data);
const mergedPropsAndData = assign({}, props, clientProps);

if (!shouldRerender && renderedElement) {
if (!shouldRerender && renderedElement && renderedElement.type === WrappedComponent) {

This comment has been minimized.

@calebmer

calebmer Mar 3, 2017

Contributor

How does this fix your problem?

This comment has been minimized.

@firstdoit

firstdoit Mar 3, 2017

Hi @calebmer! I've diagnosed and debugged this with @brunoabreu.
Basically, when a hot module replacement happens, the render method of the GraphQL is called again and has the correct, new implementation of WrappedComponent in scope.

However, because the wrapped element was already rendered, this if is true and we don't get to re-create the element using the new WrappedComponent implementation.

We initially checked it works by manually doing renderedElement = createElement... and the new component would be rendered.

I believe the intention here is to avoid re-rendering the wrapped component if the data didn't change (which is great), but the original implementation didn't consider that the Component itself might have changed (as is desired during HMR).

This basically checks that the renderedElement is up-to-date with the implementation. If the WrappedComponent didn't change (as it wouldn't without HMR), then it's referentially equal to renderedElement.type.

Or maybe we're mistaken, but this seems to be the case from what we've gathered. 😅

This comment has been minimized.

@calebmer

calebmer Mar 3, 2017

Contributor

I find it hard to believe that WrappedComponent would ever referentially change without a new instance of the GraphQL component being created 😣

Unless React Hot Loader copies over all of the properties on the old component to the new component, so a renderedElement with the incorrect type may be copied over. Can you confirm this happens?

More importantly, can you confirm that this actually fixes your issue?

If so, this looks fine to me. If we could just get a comment that reads something like this 😊:

// If we were not marked to perform a new render, and we have a previous
// rendered element of the same type as the component we are wrapping then let
// us just return that previous element instead of rendering a new one.
//
// We check the type because it may be possible with some React hot loading
// implementations that a `renderedElement` is copied over with the old
// component type. Since we do not want to render the old element again, the
// type check guarantees we render a new element in this case.

This comment has been minimized.

@firstdoit

firstdoit Mar 3, 2017

I find it hard to believe that WrappedComponent would ever referentially change without a new instance of the GraphQL component being created 😣

We too were dumbfounded by this. Here's an idea: we'll create a repo reproducing the problem and we take it from there. However it's Friday night already in Rio so we will continue on Monday. Deal? 😃

Thanks for the help!

This comment has been minimized.

This comment has been minimized.

@calebmer

calebmer Mar 6, 2017

Contributor

Thanks so much @firstdoit for the example! After some research it does look like that every time this version of React Loader reloads your component it tries to keep the same instance. So your React component instance will be === no matter the prototype. The prototype will, however, be updated as necessary to actually hot load the component. Since we don’t change out the component instance we will still have an old rendered element with an old type, so just like we thought 😊

I think this is an implementation detail specific to this version of react-hot-loader, however, this change is a logical one to make and has no negative impacts, so let’s merge 👍

@calebmer calebmer merged commit 57e5a72 into apollographql:master Mar 6, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.25%
Details

@firstdoit firstdoit deleted the vtex:fix/hot-module-replacement branch Mar 7, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 7, 2017

Released in 0.13.3 🎉

Thanks so much @firstdoit and @brunoabreu you were both a pleasure to work with 😊

@firstdoit

This comment has been minimized.

Copy link

firstdoit commented Mar 8, 2017

Likewise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment