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

Reactive variables: Updating nested properties doesn't trigger a re-render #6699

Closed
ryands17 opened this issue Jul 25, 2020 · 11 comments
Closed

Comments

@ryands17
Copy link

ryands17 commented Jul 25, 2020

Intended outcome:

Following this blog post for working with reactive variables, updating a nested value in the reactive variable doesn't re-render the React app.

import { InMemoryCache, makeVar } from '@apollo/client'
import { uuidv4 } from './utils'

export type Task = {
  id: string
  text: string
  completed: boolean
}

export type Tasks = Task[]

export const cache: InMemoryCache = new InMemoryCache({
  typePolicies: {
    Query: {
      fields: {
        tasks: {
          read() {
            return tasks()
          },
        },
      },
    },
  },
})

const initialTasks: Tasks = [
  {
    id: uuidv4(),
    text: 'Task 1',
    completed: true,
  },
  {
    id: uuidv4(),
    text: 'Task 2',
    completed: false,
  },
]

const tasks = makeVar<Tasks>(initialTasks)

export const taskOperations = {
  addtask(text: string) {
    let initialTasks = tasks()
    let task: Task = {
      id: uuidv4(),
      text,
      completed: false,
    }
    tasks([...initialTasks, task])
  },
  toggleTask(id: string) {
    let initialTasks = tasks()
    let index = initialTasks.findIndex(task => task.id === id)
    if (index !== -1) {
      tasks(
        initialTasks.map((task, i) => {
          if (i === index) {
            task.completed = !task.completed
          }
          return task
        })
      )
    }
  },
}

Here, the addTask method triggers a re-render, but the toggleTask method doesn't.

Actual outcome:

The toggleTask method should trigger a re-render

Versions

System:
OS: macOS 10.15.6
Binaries:
Node: 12.18.2 - ~/.nvm/versions/node/v12.18.2/bin/node
Yarn: 1.22.4 - /usr/local/bin/yarn
npm: 6.14.6 - ~/.nvm/versions/node/v12.18.2/bin/npm
Browsers:
Chrome: 84.0.4147.89
npmPackages:
@apollo/client: ^3.0.2 => 3.0.2

@ryands17
Copy link
Author

ryands17 commented Jul 25, 2020

So I tried updating the reactive var with Immer and it works. So are changes not detected when we use the spread(...) operator or the map function and it that bad practice with Reactive vars?

@benjamn
Copy link
Member

benjamn commented Jul 28, 2020

@ryands17 Can you show the code that issues/consumes the query (likely calling useQuery if you're using React)? Everything you've shown here looks good, so I'm at a bit of a loss. In case you were wondering, tasks(initialTasks.map(...)) should always trigger an update, because the result of the mapping is always a new array object.

My only thought is that this code

task.completed = !task.completed

should perhaps be

task = { ...task, completed: !task.completed }

to ensure the task object itself is changing (in the !== sense).

@ryands17
Copy link
Author

ryands17 commented Jul 28, 2020

I have a repo setup here that uses Immer for now.

On this line in this method:
https://github.com/ryands17/react-apollo-linkstate/blob/e06089afb4d30530701db2f533d1499e60050a23/src/config/state.ts#L53-L63

If we convert the code in the method to this:

    let initialTasks = tasks()
    let index = initialTasks.findIndex(task => task.id === id)
    if (index !== -1) {
      let newTasks = [...initialTasks]
      newTasks[index].completed = !newTasks[index].completed
      tasks(newTasks)
    }

The new state isn't reflected and the component doesn't re-render. So I guess shallow copying the variable doesn't work.

@benjamn
Copy link
Member

benjamn commented Jul 28, 2020

That code still seems to be modifying properties of the task objects in-place. Hence my suggestion about doing task = { ...task, completed: !task.completed } instead. You shouldn't need a library to do simple immutable/non-destructive updates to JS objects.

@ryands17
Copy link
Author

ryands17 commented Jul 28, 2020

Yes copying the particular task works well.
Just a side question. do reactive variables work differently and shallow copying the main variable isn't enough like we do in React's for e.g.?

@limekiln
Copy link

limekiln commented Sep 8, 2020

Could it be there is an issue with reading reactive variables anyway? Using useQuery to consume the variable, it works perfectly fine. If I read the value directly by calling the reactive variable function without arguments, no update is triggered. My update function looks as follows:

const updateVar = ({ dataVar, data }) => {
    const cachedData = dataVar();
    if (typeof cachedData === "object" && !Array.isArray(cachedData))
      dataVar(
        Object.keys(cachedData).reduce((acc, curr) => {
          if (!data) acc[curr] = null;
          else acc[curr] = data?.[curr] ? data[curr] : cachedData[curr];
          return acc;
        }, {})
      );
    else dataVar(data);
  };

My variable has the following structure:

export const myVar= makeVar({
  fieldA: { propA: null, propB: "", propC: null },
  fieldB: { propA: null, propB: "", propC: null },
  fieldC: { propA: null, propB: "", propC: null },
  fieldD: { propA: null, propB: "", propC: null },
  fieldE: null
});

Updates of field A to D, while creating a new object, do not trigger an UI update this way. Am I missing something here? The stored value is properly updated and as said before triggers an update if useQuery is used instead.

@limekiln
Copy link

limekiln commented Sep 9, 2020

I just tried using the current 3.2.0 beta and its useReactiveVar hook, that seems to work properly.

@cyril36
Copy link

cyril36 commented Dec 22, 2020

@limekiln can you show your code, i have the same issue, i have tried to use useReactiveVar but it seems that i dont use it properly

@limekiln
Copy link

As it seems, update 3.3.7 did break the useReactiveVar hook, so it seems there is no way now using reactive variables to reliably trigger a rerender.

@brainkim
Copy link
Contributor

I am reasonably certain this is fixed by #8022. Feel free to ping and yell at me if this is not the case 🤗 Should be in a release soon.

@brainkim
Copy link
Contributor

Shipped in 3.3.16

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants