tylermcginnis/re-base

syncState issue

vivekmago opened this issue · 9 comments

I am trying to update a nested object structure in FB using rebase and seems to have hit an issue with syncState such that it works for most cases except when a nested structure is involved. Is that a known issue?

What works

  + company
        + -Ksdfsdfjjhkjhsdf0X
             - name: "Test Company"  <<-- Works
             - createdAt: 1502323213122  <<--Works
             + markets <<-- Adds works at this level but removes do not
                + -Ksdjfskfdjhjh89
                   -id:-Ksdjfskfdjhjh89
                + -Kskdfjhueuhf8u
                   -id:-Kskdfjhueuhf8u
Vassi commented

I second this. When I delete a property from an object in state it doesn't get synced up.

I.E.

{
  ...
  projects: {
    m11: {
      ...
    },
    m12: {
      ...
    }
  }
}

adding the M12 object syncs, but deleting either m11 or M12 does not sync.

@Vassi I tried to reproduce this and it seems to be working for me. Which options are you using in syncState and how are you deleting the property?

Vassi commented

Hey @qwales1, thanks for looking. I just got back into the code today, here's how I'm setting up syncState (below). I was thinking about it a little over the weekend and I wonder if it's because I have multiple syncState operations going at once, that first one (allocations) works just fine.

As for deleting the property, I use the delete keyword. The state looks fine in react tools but when I refresh the page the firebase sync restores the properties that were deleted.

I'll try and create a simple repro if I can.

componentWillMount() {
    base.syncState(`${this.dbPath}/allocations`, {
      context: this,
      state: "allocations",
      asArray: false,
      then: () => this.setState({ syncedAllocations: true })
    });

    base.syncState(`${this.dbPath}/members`, {
      context: this,
      state: "members",
      asArray: false,
      then: () => this.setState({ syncedMembers: true })
    });

    base.syncState(`${this.dbPath}/memberships`, {
      context: this,
      state: "memberships",
      asArray: false,
      then: () => this.setState({ syncedMemberships: true })
    });

    base.syncState(`${this.dbPath}/phases`, {
      context: this,
      state: "phases",
      asArray: false,
      then: () => this.setState({ syncedPhases: true })
    });

    base.syncState(`${this.dbPath}/projects`, {
      context: this,
      state: "projects",
      asArray: false,
      then: () => this.setState({ syncedProjects: true })
    });

    base.syncState(`${this.dbPath}/roles`, {
      context: this,
      state: "roles",
      asArray: false,
      then: () => this.setState({ syncedRoles: true })
    });
  }
Vassi commented

Here is a repro, I admit it's entirely possible I'm mishandling the delete as I'm fairly new to react, but you can see here if you click "add" after foods it adds another item. If you refresh you'll still see the new item. If you delete one then it appears to go but if you refresh it returns.

https://codesandbox.io/s/wq8ovzkpn8

@Vassi Awesome thanks! I will take a look. Setting the property to null instead of delete I think should work how you have it.

Note:
Setting the property to null is related to firebase not React. When it syncs with firebase, firebase will delete any property with a null value and re-base calls setState on the component with the data returned.

@Vassi thanks for the repro. super helpful. It's definitely a bug. One quick fix is instead of delete set it to null and then check if the food is null before trying to render it.

removeFood(event, key) {
    this.setState(prevState => {
      var prevFoods = prevState.foods;
      let foods = { ...prevFoods };
      foods[key] = null;

      debugger;
      return { foods };
    });
  }
 const foods = Object.keys(this.state.foods).map(f => {
      let food = this.state.foods[f];
      if(!food) return null; 
      return (
        <li key={f}>
          {food.name} <a onClick={e => this.removeFood(e, f)}>remove</a>
        </li>
      );
    });

It will change the component state first and then sync up with firebase so the null check is necessary.

Vassi commented

@qwales1 I was hoping that if possible, but I'll try to work this in! Thanks for looking into it. Are you saying this is largely firebase issue then? (I had a similar issue using integer keys so firebase kept turning my objects into arrays, lol) Is it possible to workaround eventually in this library? I'd be willing to try and throw down after I'm past the time sensitive part of my project here.

The weird thing is that sometimes it deletes stuff correctly and sometimes not.

@Vassi the issue is definitely with syncState and the way syncState works with nested state. It is related to how the firebase SDK works, but the biggest problem is that using the delete keyword will not work with syncState and nested data and I think that is not very clear. The property you want to delete needs to be set to null instead. The short explanation for that is that if the property no longer exists (delete), syncState does not know that you want to change that property whether its to update the value or in this case remove it entirely.

That brings to light another issue that syncState actually behaves differently if you pass a function to setState instead of an object. The timing of when data is synced with firebase is different. In order to set a property to null safely both those cases (object or function passed to setState) need to behave in a consistent manner so you don't have to have null checks all over the place in your code. So I think the solution is to make it clear that, with syncState, if you want to delete data you do that by setting the property to null and then making sure syncState behaves in a consistent fashion to facilitate that.

That would be awesome if you want to take a shot at it. I can definitely point you in the right direction when you have time.

@qwales1 @Vassi it seems to work 100% of the time now when I try out the demo. Has something changed?