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

Add key to the items in the array (as $id) #40

Merged
merged 5 commits into from Jul 3, 2015
Merged

Add key to the items in the array (as $id) #40

merged 5 commits into from Jul 3, 2015

Conversation

puf
Copy link
Contributor

@puf puf commented Apr 26, 2015

This way the key can be used to identify the items in the React output and tie updates back to the correct child(). See this fiddle for an example (clicking a Todo item removes it): https://jsfiddle.net/frankvanpuffelen/s27nz1hb/

puf added 3 commits April 26, 2015 15:39
This way the key can be used to identify the items in the React output and tie updates back to the correct `child()`
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 98.33% when pulling f098f7c on puf:puf-add-id-to-array-items into e3bbf46 on firebase:master.

@jwngr
Copy link

jwngr commented May 5, 2015

Thanks for the PR @puf! Have you seen the discussion in #37? I think we should go with a more fully-fledged solution here. As noted in the issue, I'd like to do something very similar to what we do with AngularFire. Most notably, your solution won't work for primitive values since setting $id on the primitive value won't actually have the effect we want.

@puf
Copy link
Contributor Author

puf commented May 22, 2015

@jwngr quick double check here....
Case 1: array of objects; if you pass in:
firebaseRef.set({ first: { index:1 }, second: { index:2 }, third: { index:3 } });
you get:
{"items":[{"index":1,"$id":"first"},{"index":2,"$id":"second"},{"index":3,"$id":"third"}]}

Case 2: array of primitives; you pass in:
firebaseRef.set(["first", "second", "third"]);
You now get:
{"items":["first","second","third"]}
You propose:
{"items":[{$id: 0, $value: "first"}, {$id: 1, $value: "second"}, {$id: 2, $value: "third"}]}

That it? Or am I missing something? Test cases most welcome. :-)

@jwngr
Copy link

jwngr commented May 27, 2015

Yup, that's pretty much it. That is what we do with AngularFire and I think it works out pretty well.

@puf
Copy link
Contributor Author

puf commented May 28, 2015

OK. I got that one working easily, but of course then some of the original
tests that use primitives are failing (since they don't do .$value). I'll
change those and update the PR.

On Wed, May 27, 2015 at 3:03 PM, Jacob Wenger notifications@github.com
wrote:

Yup, that's pretty much it. That is what we do with AngularFire and I
think it works out pretty well.


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

@jwngr
Copy link

jwngr commented Jun 1, 2015

Sounds good. We definitely do need to update the test suite and will also probably need to list this as a breaking change. I'm totally up for doing that though since this is strictly improved functionality.

@jwngr jwngr assigned puf and unassigned jwngr Jun 1, 2015
@puf puf assigned jwngr and unassigned puf Jun 2, 2015
@puf
Copy link
Contributor Author

puf commented Jun 2, 2015

@jwngr Update the tests and fixed a few more case. Back to you.

@AndrewBestbier
Copy link

Thank you for this!

@jwngr jwngr merged commit 2c01492 into FirebaseExtended:master Jul 3, 2015
@jwngr
Copy link

jwngr commented Jul 3, 2015

Thanks for this PR @puf! I made a few changes myself (renamed $id to $key and fixed issue with sparse arrays) and then merged this in. Time to crank out a few more fixes for other issues and get an updated version of ReactFire out there.

@FirebaseExtended FirebaseExtended locked and limited conversation to collaborators Dec 9, 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.

None yet

4 participants