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

Inconsistency in data retrieving #105

Closed
lisamburns opened this issue Feb 25, 2016 · 20 comments
Closed

Inconsistency in data retrieving #105

lisamburns opened this issue Feb 25, 2016 · 20 comments

Comments

@lisamburns
Copy link
Contributor

There's tons of calls to retrieve data from the API but the app is very inconsistent in terms of how information is passed, where API calls to fetch data are made, how the views update themselves. Can we discuss the advantages/disadvantages and standardize it? Not sure what pattern to follow because there's so many in the app. Also it just makes it harder to work with when it's not consistent.

  1. Sometimes there is an APIWorker, sometimes there isn't.
  2. Some (Opinions.jsx) directly make AJAX calls within the component, passing in setState.
  3. Others (Ballot.jsx) call a Store initialize or retrieve method, passing in setState. Store's method in turn calls APIWorkers, eventually resolving promise and calling the callback(the setState).
  4. Others (PositionItem.jsx) call a Store retrieve method. Store calls APIWorkers, which fire an action when the data is retrieved. AppDispatcher register event, updates the store and emits change, which updates the component because component has registered changeListener.
  5. There are other minor variations of these patterns....
@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

Yeah Lisa, I agree.

The initial work patterns have been variable, as you noticed. I’ve been experimenting and prototyping methods/patterns. I think its best for us to navigate away from the initialize pattern like you've mentioned and start breaking the components apart and decoupling them so they are more modular. I can check in a branch when I think I have some stuff to work with but I'm still experimenting with these features.

There’s a lot of architectural work that needs to be done in my opinion; Backend and front-end.

Should we discuss this on Saturday?

Nick

On Feb 24, 2016, at 8:03 PM, Lisa Cho notifications@github.com wrote:

There's tons of calls to retrieve data from the API but the app is very inconsistent in terms of how information is passed, where API calls to fetch data are made, how the views update themselves. Can we discuss the advantages/disadvantages and standardize it? Not sure what pattern to follow because there's so many in the app.

Sometimes there is an APIWorker, sometimes there isn't. It makes it harder to work with.
Some (Opinions.jsx) directly make AJAX calls within the component, passing in setState).
Others (Ballot.jsx) call a Store initialize or retrieve method, passing in setState. Store's method in turn calls APIWorkers, eventually resolving promise and calling the callback(the setState).
Others (PositionItem.jsx) call a Store retrieve method. Store calls APIWorkers, which fire an action when the data is retrieved. AppDispatcher register event, updates the store and emits change, which updates the component because component has registered changeListener.
There are other minor variations of these patterns....

Reply to this email directly or view it on GitHub #105.

@DaleMcGrew
Copy link
Member

@nf071590 Can you create a brief cheat sheet of the WebApp code that you think we should be emulating for now, vs what we shouldn't? For example, you had said that we were moving away from API Worker, and that was helpful for me to know.

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

I think that Opinions should generally make a network request every time a user navigates to the Opinions route when the store doesn't have 20 orgs in it... This way the user can grab a more Orgs to follow or ignore and remove them from the Org store.... Does that make sense?

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

@DaleMcGrew Yeah, i'm working on some framework level stuff, been experimenting and prototyping with FluxStore and I will let you know when I have what I think is a good pattern. I'm not sure if there is a 1 size fits all answer to the project but I'll be brainstorming more today...

I wasn't expecting everyone to follow the same pattern of BallotStore initially, I was trying to get something up and working, and it was a childlike attempt at linking Flux and React with the network. Now that we have a deeper understanding of React and Flux, we should be able to refactor to break components apart and modularize them.

I'm trying to figure out the best way to slip network calls into the Action --> Dispatcher --> Event graph... It seems like it would go in between the Action/Dispatcher. So I want to break apart the Flux architecture so that we can slip our endpoints into it and our Action/Dispatcher system can be smart enough to grab data from the server when it needs to, then store it, then return it to the component... I have something like this working on my local but it isn't necessarily that pretty.

But then on top of that I'd like a way for us to be able to create components that are wrapped in their own action Action/Dispatcher/Store... Probably using the decorator pattern I've seen before...

I still need to clean this stuff up and try to figure out what is the optimal solution.

Also creating Action/Dispatcher dependencies is another major key. I'm trying to figuring out how Facebook engineers use the waitFor method to create dependencies between stores, this is important and I need to work on figuring out that aspect of their architecture I believe.

If anyone has input on this, please don't be shy.

@DaleMcGrew
Copy link
Member

@nf071590 I agree we should "freshen up" the data frequently, yes. Ideally when you retrurn to the "More Opinions" page, we would display the orgs that are already in the local VoterGuideStore before retrieving more, so the "Loading" swirl shows up at the bottom of the existing data, and voters don't have to wait. PS. great work on the Opinions page Nick!

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

@DaleMcGrew thanks Dale!

@lisamburns
Copy link
Contributor Author

Thinking about this more and also listening to some talks, I like using the idea of waitFor, and thinking of data loading as being caused by user actions (just the same as the user action FOLLOW or UNFOLLOW). IE would could have the action START_APP, which is fired by just loading the thing. The VoterStore could subscribe to the event and fetch the Voter. The BallotStore could fetch the Ballot. The Dispatcher could waitFor the Voter to finish fetching and then tell the BallotStore to fetch the Ballot. (Since it's possible there's more than one ballot, we may also want to separate out a Candidate and MeasureStore, which could then start fetching its items once BallotStore finishes fetching.). Another action could be VIEW_CANDIDATE, which causes individual positions to be fetched into the position store.

@lisamburns
Copy link
Contributor Author

My other comment is that a lot of dependencies are caused by the structure of the API (i.e. some only accept regular ID and type (CANDIDATE or MEASURE), whereas we only have WE_VOTE_ID. Hence I'd try to get the ID and type from the BallotStore to call an API and it would be empty, so I'd have to wait for something to finish fetching to get the ID and TYPE from the store, and then call the API. If the API accepted we_vote_id uniformly, then no waiting (at least in this case) would be required.

@DaleMcGrew
Copy link
Member

Thank you @lisamcho, I am in the process of making sure all API endpoints accept we_vote_id as an input variable. If you need any in particular now, please let me know.

@lisamburns
Copy link
Contributor Author

Another idea I heard at a tech talk and liked that I haven't used myself is separating presentational vs container components. A lot of my components initialize their state to what they get by props, then setState onChange. The other idea is that you try to get as much of the data connectivity into the state of a container component (all fetching and setting of State), and then the subcomponents only need to receive and sdisplay their props.

@lisamburns
Copy link
Contributor Author

@DaleMcGrew yes that would be great!

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

I think I found a really clean way to approach the initialization problem... I can check this into a new branch if anyone is interested in taking a look...

@lisamburns
Copy link
Contributor Author

I'd like to see! @nf071590

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

@lisamcho checkout the new flux-prototype branch, ballots are the only thing that has changed and I did not do any additional work but to load additional data etc. It only does one request to populate page.

checkout the files
src/js/dispatcher/dispatcher.js (to see the dispatcher prototype extension)
src/js/actions/BallotActions.js (to see the BallotAction.init definition, which is called in the src/js/routes/Ballot/Ballot.jsx when componentDidMount is invoked )
src/js/stores/BallotStore.js (which sets the state of the Store and emits a change back to the ballot)
src/js/routes/Ballot/Ballot.jsx (to see where it listens for the change and then updates the state and renders the view)

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

What I like about this approach is that it allows stores to subscribe to network requests and populate when they return... Also network errors,which could probably prove to be useful... Ex:

try{ Action.init() } catch (e) { // ERROR LOADING BALLOT // handle error logic at ui level }

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 25, 2016

Or possibly subscribe components to error events and trigger them on a network error is another possibility

@DaleMcGrew
Copy link
Member

@nf071590 This sounds like a good approach. Great work!

@lisamburns
Copy link
Contributor Author

Hey this is really great! Having the ajax utility take care of creating an action when it returns with name of API endpoint is genius, that will save us a lot of time. I'm onboard with moving in this direction. What about you guys? @pertrai1

@fi0rini
Copy link
Collaborator

fi0rini commented Feb 27, 2016

The wait for method can hopefully help with endpoint dependencies but I'm
not sure how.
On Feb 27, 2016 11:11 AM, "Lisa Cho" notifications@github.com wrote:

Hey this is really great! Having the ajax utility take care of creating an
action when it returns with name of API endpoint is genius, that will save
us a lot of time. I'm onboard with moving in this direction. What about you
guys? @pertrai1 https://github.com/pertrai1


Reply to this email directly or view it on GitHub
#105 (comment).

@DaleMcGrew DaleMcGrew added this to the Version 0.8 - Refactoring milestone Feb 27, 2016
@pertrai1
Copy link
Member

pertrai1 commented Mar 6, 2016

The major refactor from @lisamcho and @nf071590 should handle this issue and if there are more that are needed, a new issue can be opened up that is based on the refactoring from that change.

@pertrai1 pertrai1 closed this as completed Mar 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants