Skip to content

Conversation

@davepwsmith
Copy link
Contributor

@davepwsmith davepwsmith commented Feb 9, 2020

Had some spare time and thought I would (try to) contribute instead of just moaning on the issues :-)

This is part of the way to #137

Not sure if this is the most efficient way to do this, but it fits in well with the general pattern.

Presume you will need me to document it (although it will mostly be a copy and paste I should think!!) - I'll do that next week.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@davepwsmith davepwsmith requested a review from jhuleatt February 9, 2020 23:11
@jhuleatt
Copy link
Collaborator

@davepwsmith thank you for reviving #137 and for the thorough PR with tests and sample code!

My main question is whether we instead want to handle this in ReactFireOptions with a once param or similar instead of writing out dedicated *Once functions. I'm meeting with @davideast and @jamesdaniels later today, so I'll run this by them too. Any thoughts on the tradeoffs?

@davepwsmith
Copy link
Contributor Author

I don't really see that there is a meaningful difference between those two approaches, from the point of view of a user of the library (of which I am one!) - I would point out that the names of the hooks are getting pretty long! I guess it could be one less thing to import, but I haven't often found that I am using multiple hooks in one component where I can avoid it - not least because #202 precludes passing data between them easily.

I also wanted to take a look at doing the same for collections, but somehow I wasn't sure it seemed as simple. I was using the sample code partly as a way of working out when the observables were returning (the hooks seem to return a value more than once which is interesting... I guess that is the component getting re-rendered, I didn't look into it too hard) and when I tried the same approach with collections on the face of it, it seemed to return the observable every time the collection was modified - still with the first value, but nonetheless, not "Once". I might take another look at that and check that it isn't the sample code itself that is causing that though.

Let me know any further thoughts.

Copy link
Collaborator

@jhuleatt jhuleatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davepwsmith thanks again for the very thorough PR and quick follow-up to comments! It's a huge help to have you work on these once functions, and after chatting with @davideast we're in agreement that this is just about ready to merge.

We'd like to wait to merge this PR until #210 is merged into master, because that is a pretty major refactor of the inner workings of the library and we want to avoid as many merge conflicts as possible. Also, #210 changes how we create the observableId (example), and it is a pattern we should use on these functions too. This would look like:

  • useFirestoreDocOnce:${ref.path}:${JSON.stringify(options)}
  • useFirestoreDocDataOnce:${ref.path}:${JSON.stringify(options)}

Speaking of those observableIds, I think this is actually related to the trouble you had implementing once for collections. We had a collision in observableId in the useFirestoreCollection* functions that lead to some values being reused where they shouldn't have been (useFirestoreCollection using useFirestorecollectionData's values and vice versa). #210 also fixes that!

@jamesdaniels jamesdaniels dismissed their stale review February 12, 2020 00:09

My concerns were addressed.

Copy link
Collaborator

@jamesdaniels jamesdaniels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamesdaniels
Copy link
Collaborator

Thanks for the contribution @davepwsmith! Look for it in the next release.

@jamesdaniels jamesdaniels merged commit ca32a93 into FirebaseExtended:master Feb 12, 2020
@FirebaseExtended FirebaseExtended locked and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants