Skip to content

bindFirebaseRef is not idempotent #306

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

Closed
iamareebjamal opened this issue Jul 21, 2019 · 5 comments
Closed

bindFirebaseRef is not idempotent #306

iamareebjamal opened this issue Jul 21, 2019 · 5 comments

Comments

@iamareebjamal
Copy link

In 2.3.0, bindFirebaseRef was idempotent. We could simply call it in component created and call it a day. No matter how many times that page content was displayed, it showed the data fetched first time only

https://deploy-preview-308--roboclub.netlify.com/news

Switch between projects, team, and news. Only once the data is fetched and shown. And switching the navigation is as expected where the previously loaded data is retained.

After upgrading to 3.1.0, not only the data is removed and hence refetched, the bound property is first set to null, which breaks the contract of getters assumption of pre-existing data.

https://deploy-preview-323--roboclub.netlify.com/news

Try switching between projects, news and team and notice every time, data is refetched. And also, team page is broken after switching 2nd time because team property is set to null.

This forces developers to write code like this

const state = () => { team: {} }

setTeam: firebaseAction(...) => {
  if (state.team.length) return
  bindFirebaseef(...)
}

This is needless defensive coding and error prone as it depdends on state value. Also, since team is set to null, the condition changes to state.team && state.team.length which is all the more ugly.

Furthermore, checking the state before setting reference disables any chance of changing the reference by adding filter, etc as in both case there'll be data in state and you can't return in that case. So, an extra property is needed to store which reference is currently bound to the state.

Note that this issue is not same as previously open issues as they are from about 2017 or so while this behaviour was introduced in 3.x and 2.3.0 was working fine as shown in the demos above

@iamareebjamal
Copy link
Author

@iamareebjamal
Copy link
Author

Actually, works for objects but not for arrays. So a valid issue

@iamareebjamal iamareebjamal reopened this Jul 21, 2019
@posva
Copy link
Member

posva commented Jul 21, 2019

This is tracked at #83
With arrays, they get reset to an empty array first because we push to that array. I think it will make sense to reuse the reset option instead of introducing a new one

@posva posva closed this as completed Jul 21, 2019
@iamareebjamal
Copy link
Author

@posva But it was working fine in 2.3.0 while #83 is open since 2017?

@posva
Copy link
Member

posva commented Jul 22, 2019

It was removed so it has to be added back! It's a breaking change that wasn't listed because I do plan to add it but I should indeed mention

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

2 participants