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

mutations on array do not lead to rerender #1060

Closed
3 tasks done
qinhuaihe opened this issue Oct 26, 2018 · 14 comments
Closed
3 tasks done

mutations on array do not lead to rerender #1060

qinhuaihe opened this issue Oct 26, 2018 · 14 comments
Labels
not a bug Seems like a bug, but actually it is not should be ready to close Should be ready to close soon if nothing else comes up

Comments

@qinhuaihe
Copy link

qinhuaihe commented Oct 26, 2018

Bug report

  • I've checked documentation and searched for existing issues
  • I've made sure my project is based on the latest MST version
  • Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

reproduction

please look at src/page/Test where the store and the page located. I want to provide a codesandbox. But there are too many modules to create one. I'm sorry.

Describe the expected behavior

When I change the array, the page should rerender.

Describe the observed behavior

When I push some item into an array, the array changed, but there is no rerender. When I change the text in the same model, the rerender happend. What's wrong with these code?

@qinhuaihe
Copy link
Author

This is the page, data array works on the div and the Table with some difference.

page code

import { Button, Card, Table } from 'antd';
import { inject, observer } from 'mobx-react';
import * as React from 'react';
import { ITestStore } from './stores/test';

export interface ITestProps {
  testStore: ITestStore,
}

@inject(({ stores }) => ({
  testStore: stores.test,
}))
@observer
export default class Test extends React.Component<ITestProps> {
  public render() {
    const { testStore } = this.props;
    const { text, data } = testStore;
    return (
      <Card>
        <Button onClick={testStore.randomText}>randomeText: {text}</Button>
        <Button onClick={testStore.addItem}>addItem</Button>
        {/* when I add an div here, everything works ok */}
        {/* <div>{JSON.stringify(data)}</div> */}
        <Table
          size="small"
          columns={[{ title: 'Name', dataIndex: 'name' }]}
          dataSource={data}
          pagination={false}
        />
      </Card>
    );
  }
}

@qinhuaihe qinhuaihe changed the title mutations on array do not lead rerender mutations on array do not lead to rerender Oct 26, 2018
@xaviergonz
Copy link
Contributor

xaviergonz commented Oct 26, 2018

That is more about mobx/mobx-react than mobx-state-tree

Basically what happens is this

Premises:

  • Table is a PureComponent, which means that if none of the props change between re-renders it won't re-render itself

  • observer for Test will make sure the component is only re-rendered when any of the TOUCHED values inside render change (basically a mobx-autorun on render that forces an update if any observable value TOUCHED (accessed) when render was last run is changed)

(json.stringify commented out)

  • Test only touches data (the reference to the array, which never changes) and Table doesn't touch the data inside the array again since it is a PureComponent
  • result, no re-render

(json.stringify not commented)

  • the json.stringify function internally touches (accesses) all array and inner array objects in order to generate a json representation of it
  • therefore any time the array or anything inside it changes the Test component (and therefore its Table child) will re-render

Your best bet is either:
a) changing the Table component so it uses @observer rather than PureComponent, so it touches the actual data inside the array upon rendering (more performant)
b) if that's not possible then to change the Test render method so it touches all the data inside the array (e.g. traversing all its items and inner items, you don't need to actually render them). A quick but inefficient way to do this is moving the JSON.stringify to the top of the render method. Either way this solution will be less efficient than A.

There are of course more solutions, but those two are the simplest ones that come to my mind

@xaviergonz xaviergonz added not a bug Seems like a bug, but actually it is not should be ready to close Should be ready to close soon if nothing else comes up labels Oct 26, 2018
@qinhuaihe
Copy link
Author

Thanks for your reply, you are so kind.
But I checked the code of Table, it is a Component instead of a PureComponent. I'm confused.

@xaviergonz
Copy link
Contributor

Sorry I was looking at StandardTable

still Table seems to have lots of custom logic to achieve something similar

@xaviergonz
Copy link
Contributor

xaviergonz commented Oct 26, 2018

Another way is to do
<Table dataSource={getSnapshot(data)} .../>,
and it will work since it will generate a whole new reference (JSON object) every time it is rendered (everytime the array or any of its inner items change), but again this will also be inefficient

@qinhuaihe
Copy link
Author

I created an minimal reproduction codesandbox for testing these cases.
Then I found the behaviors are so weird.
It seems that I have to use 'getSnapshot' everytime when using the array from mobx-state-tree.

@xaviergonz
Copy link
Contributor

If you add @observer to ListComponent the first sample will also work (and be more performant)

@xaviergonz
Copy link
Contributor

Maybe you can try

const OTable = observer(Table)

and then use OTable instead of Table and that will work without having to make snapshots, but that very much depends on the implementation of Table itself

@mkramb
Copy link

mkramb commented Oct 27, 2018

I had similar problem, I think the last suggestion is the best - yes, it's not pretty. You could also always pass a new array to Table component as data.slice() and then rely on rows keys for React reconciler to update only new rows.

In my case I changed the ”Table” component API to something like https://material-ui.com/demos/tables/, where you need to loop through all rows in your render function (which means it will be tracked by mobx).

@qinhuaihe
Copy link
Author

Ok, I understand this is a feature instead of a bug now.

@qinhuaihe
Copy link
Author

mobxjs/mobx#101

@xaviergonz
Copy link
Contributor

Glad you got it solved :)

sfentress added a commit to concord-consortium/geocode that referenced this issue Apr 18, 2019
Since these components are passed an observable array, they won't
re-render automatically when the array's internals update, and instead
only re-render when a primitive value updates. This means that their
rendering is always a step behind in the simulation.

This makes the less pure-componenty, but seems to be the recommended way.
See:

mobxjs/mobx-state-tree#1060
https://github.com/mobxjs/mobx-react#faq
mobxjs/mobx#101

An alternative would be to use `getSnapshot(data)`, but this is claimed
to be less efficient. (Untested, and perhaps for our use-case is negligible
and the cleaner code would be preferred.)
@schumannd
Copy link

schumannd commented Dec 3, 2019

@zmayor can you explain what you mean by "it is a feature now"?

I have run into this problem. Now as a hacky hack to trigger rerenders, we add stuff like.

this.myArray = this.myArray.filter(_obj => true);

To all our mutating functions. This can't be intended right?

Also, the link to your reproduction is dead now. Can you post it again? I would be very interested in it

@mweststrate
Copy link
Member

@schumannd that is definitely not how it is supposed to be done. But please don't comment on issues that are closed for over a year, but rather file a new bug report.

@mobxjs mobxjs locked as off-topic and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
not a bug Seems like a bug, but actually it is not should be ready to close Should be ready to close soon if nothing else comes up
Projects
None yet
Development

No branches or pull requests

5 participants