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

bug(reducers): updates to arrays inside documents don't work as expected #140

Closed
pdyxs opened this issue Sep 25, 2018 · 9 comments

Comments

Projects
5 participants
@pdyxs
Copy link
Contributor

commented Sep 25, 2018

Do you want to request a feature or report a bug?
bug

What is the current behavior?
If you store an array inside a firestore document (e.g. { content: ['an', 'array'] }) and update that document such that the array is shorter ({ content: ['single'] }), the update action that fires merges the arrays and doesn't delete the extra elements ({ content: ['single', 'array'] }).

Note that { content: ['single'] } is what is in the firestore database at this stage.

What is the expected behavior?
For the array to be pruned

Which versions of dependencies, and which browser are affected by this issue? Did this work in previous versions or setups?
Definitely exists in 0.5.8, unclear if it worked previously

Steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.
See pull request and the test attached.

@GuillemotF

This comment has been minimized.

Copy link

commented Sep 27, 2018

I have a similar behaviour with objects in a document (instead of an array) when removing a property.

Example :

const documentUpdate = {};
documentUpdate['someObject.someProperty'] =  firestore.FieldValue.delete();
firestore.update(
      {
        collection: 'test',
        doc: 'someId',
      },
      documentUpdate,
);

Will correctly update data state in redux but not the ordered one.

Edit : Your PR also fixes this issue

@prescottprue

This comment has been minimized.

Copy link
Owner

commented Oct 1, 2018

@GuillemotF The current issue with this fix has to due with how subcollections will no longer merge onto document objects. I'm currently working on a new pattern for redux state where subcollections will not be stored on the doc directly.

That said, I'm open to merging this change to a next release, we will just need to make sure we note that it will be breaking for some.

@prescottprue prescottprue changed the title Updates to arrays inside documents don't work as expected fix(reducers): updates to arrays inside documents don't work as expected Oct 1, 2018

@prescottprue prescottprue changed the title fix(reducers): updates to arrays inside documents don't work as expected bug(reducers): updates to arrays inside documents don't work as expected Oct 1, 2018

@prescottprue prescottprue added the bug label Oct 1, 2018

@prescottprue

This comment has been minimized.

Copy link
Owner

commented Oct 2, 2018

Started the v1.0.0 Roadmap doc which includes notes about the current plans for the new state pattern.

It seems like we might be able to release this under a flag before 1.0.0 so that folks can use it in the meantime.

@prescottprue prescottprue referenced this issue Oct 2, 2018

Merged

v1.0.0-alpha #142

7 of 8 tasks complete
@GuillemotF

This comment has been minimized.

Copy link

commented Oct 2, 2018

@prescottprue Thanks for these explanations, I think that separating the sub-collections from the documents is the only "Firestore's way" to do since it is also possible to have a property of a document of the same name as a sub-collection.

Maybe you can have a compromise like this :

// Data
{
  projects: {
    myProject: {
      subCollections: {
        events: {
          ABC123: {
          
          }
        }
      }
    }
  }
}
// Ordered
{
  projects: [
    {
      id: 'myProject',
      subCollections : {
        events: [
          {
            id: 'ABC123'
          }
        ]
      }
    }
  ]
}

But it may be hard to make this working properly.
It still locks one property (subCollections here) and may not correct actual issues, so the design in your roadmap seems the only consistent one for me.

Btw I'm already using storeAs for every subCollection I use, I think this is a very good way to structure and personalize your redux store.

@prescottprue

This comment has been minimized.

Copy link
Owner

commented Oct 2, 2018

@GuillemotF The structure proposed in the Roadmap stores the subcollections by the parent document key still, it is just not under the same doc object within redux:

// Data
{
  projects: {
    myProject: {
      createdBy: "me"
    }
  },
  "projects/myProject/events": {
    ABC123: {
      event: 'data'
    }
  }
}

// Ordered
{
  projects: [
    {
      id: "myProject"
    }
  ],
  "projects/myProject/events": [
    {
      id: "ABC123",
      event: 'data'
    }
  ]
}

prescottprue added a commit that referenced this issue Oct 3, 2018

v1.0.0-alpha (#142)
* feat(reducers): `ordered` and `data` reducer using new v1 state pattern outlined in [the v1.0.0 roadmap](https://github.com/prescottprue/redux-firestore/wiki/v1.0.0-Roadmap) (full query path in ordered, sub-collections separate from doc in data)
* feat(core): `firestoreDataSelector ` and`firestoreOrderedSelector` utilities for selecting values from state
* fix(reducers): `LISTENER_RESPONSE` action not correctly updating state for sub-collections - #103
* fix(reducers): `ordered` state not updated with added item in array - #116
* fix(reducers): updates to arrays inside documents don't work as expected - #140
@compojoom

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

That looks like a good change. Another side-effect of the current way we store the subcollection is that whenever you try to update the parent document and you read the document from redux - you also get the subcollection as property on the doc... And if you try to store the parent document - you also store the subcollection as prop on the document, which is often not desired.

@prescottprue

This comment has been minimized.

Copy link
Owner

commented Oct 5, 2018

@compojoom Exactly! Definitely another reason behind the change. Glad to have others looking into it since it is such a large change.

Since getting from state with the new syntax could possibly be confusing, I included some state selection utilities called firestoreDataSelector and firestoreOrderedSelector.

Interested to hear input on how using the new syntax feels, so if anyone gets a chance to check it out, post your thoughts. Totally open to renaming things, adding more utilities, or changing their syntax - the goal is to make it as easy to use as possible while still making it work will with scaled setups.

@prescottprue prescottprue added this to To do in v0.7.* Jan 15, 2019

prescottprue added a commit that referenced this issue Feb 19, 2019

v0.7.0
* fix(orderedReducer): updating arrays within a document properly - #103, #140, #141 - @pdyxs
* fix(query): oneListenerPerPath takes into account storeAs - #144 - @compojoom
* fix(orderedReducer): deleting a sub-collection no longer causes TypeError - #161
@prescottprue

This comment has been minimized.

Copy link
Owner

commented Feb 19, 2019

v0.7.0 release includes a fix for updating array items. Thanks to all for reporting!

v0.7.* automation moved this from To do to Done Feb 19, 2019

@YuvalKlein

This comment has been minimized.

Copy link

commented Feb 19, 2019

Yeesss!! working!
thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.