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

composer function re-run after every prop change #2

Open
arunoda opened this issue Jan 4, 2016 · 18 comments
Open

composer function re-run after every prop change #2

arunoda opened this issue Jan 4, 2016 · 18 comments

Comments

@arunoda
Copy link
Owner

arunoda commented Jan 4, 2016

Currently, composer function will re-run for every prop change. This is a continuation of the discussion started at: facebook/react#3398 (comment)

@arunoda
Copy link
Owner Author

arunoda commented Jan 4, 2016

@oztune Okay, you suggested to return a function and a key from the composer function and function will re-run if the key is different from the last time.

I assume here's the reason for this:

  • Your data fetching code (the logic inside the component) is live inside another file.
  • It may change the need of props time to time.
  • So, if we go with the option I suggested (mention props, when composing the component),
  • If I need to use a new prop, I need to change in two places. That may leads to errors.
  • I can't change the re-run logic runtime.

So, I get it.
I like to add this rather making it something mandatory. If someone is getting some issues with this, he can go with that.

Just like how we implement shouldComponentUpdate.

What do you think?

@ccorcos
Copy link

ccorcos commented Jan 5, 2016

@arunoda, don't you think that this is still imperative programming? You've isolated the side-effect to a container component, but your entire UI isnt side-effect free anymore and is imperatively tied to the lifecycle of the components.

Take this analogy. We used to do this:

turnOn = (e) => {
  e.target.addClass('on')
}
turnOff = (e) => {
  e.target.removeClass('on')
}

And now we do this:

render = (state, props) => {
  return <div className={state.on ? 'on' : 'off'}></div>
}

The the second example, we just declare what we want and let React parse the component tree and handle any mutations.

The way you're proposing to fetch data in containers (and which I think just about everyone is doing) looks a lot like this:

componentWillMount = () => {
  fetchData()
}
componentWillUnmount = () => {
  cleanup()
}

This is the same imperative style as before. What I think we should be doing is declaratively specifying what is needed and letting some other service parse and do all the imperative heavy lifting.

Relay is a step in the right direction. But I think its pretty convoluted how the Relay container talks to the React component and how the Relay container data gets accumulated into one declarative top-level request.

Just like how we bind callback functions to vdom in the render function, what if we did the same for declarative data fetching?

fetch = (state, props) => {
  return {
    query: someGraphQLQuery,
    onData: (data) => this.setState({data}),
    onError: (error) => this.setState({error})
  }
}

fetch is run everytime render runs and is diffed just like the vdom is diffed. If some request disappears, then its cleaned up automatically. If something is already fetched, then it doesnt re-fetch it.

@arunoda
Copy link
Owner Author

arunoda commented Jan 5, 2016

This project is not something I propose to introduce declarative data fetching. But just create a shorthand method for what everyone do it right now.

declaritive data fetching needs to done in the caching layer. But it should not need to tie up with any UI layer. This is how we do it in our meteor-graphql example: https://github.com/kadira-samples/meteor-graphql-demo/blob/master/client/containers/blog_post.jsx

Even though component, re-render it won't fetch data again. This can be true for Meteor as well, with subs-manager like projects.

I believe in multiple data stores which manages the state for our apps. That's why I started this project.

@ccorcos
Copy link

ccorcos commented Jan 5, 2016

Yeah, thats the same pattern I used when I built findashindig.com: https://medium.com/@chetcorcos/shindig-react-data-component-aa0d2c82059e#.r2jveywu7

If you're using GraphQL, then it seems like you should compose your queries all the way up to the top-level and have a single container for all your data. Then everything inside the container is just the UI and its pure 👍

@arunoda
Copy link
Owner Author

arunoda commented Jan 5, 2016

It's upto the user. Its not 100% possible, but can do it.
Specially for apps which only render data.

@ccorcos
Copy link

ccorcos commented Jan 5, 2016

Why isnt it possible?

@arunoda
Copy link
Owner Author

arunoda commented Jan 5, 2016

In apps, we may have to load data from other data sources not just from GraphQL. Then we need to deal with them.

@venturecommunism
Copy link

Does JSON-LD have any applications to this problem?

@patrickml
Copy link

I seem to be having the same issue where shallow compare isn't helping at all.

http://cl.ly/1V0Y1r0m3f0c

in this video, you can see that the props do not change as I am returning the same data every time. With that being said you can also see that using the react console that the components are re-rendering. Any ideas why?

@clayne11
Copy link
Contributor

Can you post the repo here so we can run it for ourselves?

@patrickml
Copy link

Sadly I cannot,

I will try and make a repo to replicate the issue however — it will be sometime this weekend.

@sahanDissanayake
Copy link

Anything new with this ?

I have facebook like layout. 100 posts 1 like button on each post. when one like button is clicked. the composer function for the like button is running 100 times before the like actually takes place.. it sucks.

@sahanDissanayake
Copy link

Not sure if I'm having this same issue but the one i have is an issue. Once the data gets big enough there is a huge lag because of all the computations

Anyone ever come across this ?

@sahanDissanayake
Copy link

Is this the only solution ? https://atmospherejs.com/meteor/react-meteor-data

@arunoda
Copy link
Owner Author

arunoda commented Jun 27, 2016

@sahanDissanayake your issue is not related to this. It seems like a reactive issue.
Just create a new issue with a sample repo.

@sahanDissanayake
Copy link

sahanDissanayake commented Jun 27, 2016

Sorry to mention my issue everywhere.. I will create a new repo in another 4 hours or so and will create a new issue and ping you. Thanks

I did some bad coding and hacking and limited the re-renders using shouldComponentUpdate.. But im sure there is a better solution

@markshust
Copy link
Contributor

Can someone post a sample repo to reproduce the problem? I'm experiencing random issues with react-komposer, and everything seems to be leading back to this ticket.

@clayne11
Copy link
Contributor

We have a PR (#99) waiting to be pulled in that completely fixes this issue.

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

7 participants