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

syncState issue #227

Open
vivekmago opened this issue Aug 16, 2017 · 9 comments
Open

syncState issue #227

vivekmago opened this issue Aug 16, 2017 · 9 comments

Comments

@vivekmago
Copy link

vivekmago commented Aug 16, 2017

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
Copy link

Vassi commented Feb 8, 2018

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.

@qwales1
Copy link
Collaborator

qwales1 commented Feb 9, 2018

@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
Copy link

Vassi commented Feb 12, 2018

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
Copy link

Vassi commented Feb 12, 2018

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

@qwales1
Copy link
Collaborator

qwales1 commented Feb 12, 2018

@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.

@qwales1
Copy link
Collaborator

qwales1 commented Feb 19, 2018

@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
Copy link

Vassi commented Feb 19, 2018

@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.

@qwales1
Copy link
Collaborator

qwales1 commented Feb 19, 2018

@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.

@nebrelbug
Copy link

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

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

4 participants