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

Warn about reusing object within attrs? #2826

Open
gjmcn opened this issue Apr 2, 2023 · 3 comments
Open

Warn about reusing object within attrs? #2826

gjmcn opened this issue Apr 2, 2023 · 3 comments
Labels
Type: Question For issues that are purely questions about Mithril, not necessarily bug reports or suggestions

Comments

@gjmcn
Copy link

gjmcn commented Apr 2, 2023

Mithril.js version: 2.2.2

Browser and OS: Chrome, Windows 10

The following had me confused for some time:

let style = {
  width: '100px',
  height: '30px',
  background: '#4682B4',
};

m.mount(document.body, {
  view: () => [
    m('input', {
      type: 'range',
      min: 1,
      max: 200,
      oninput: function() { style.width = `${this.value}px` }
    }),
    // m('div', { style })            // does not trigger redraw
    // m('div', { style: style })     // does not trigger redraw
    m('div', { style: { ...style }})  // works
  ]
});  

I think the issue is that the // does not trigger redraw versions are resuing an object within attrs and I guess this is not allowed? If so, the problem is that (unlike when reusing attrs itself) Mithril does not give a warning for the above case. Perhaps it should? Or if this is too inefficient, maybe this could be covered more in the docs? - apologies if it is and I missed it!

@gjmcn gjmcn added the Type: Question For issues that are purely questions about Mithril, not necessarily bug reports or suggestions label Apr 2, 2023
@CreaturesInUnitards
Copy link
Contributor

CreaturesInUnitards commented Apr 2, 2023

@gjmcn here (in your two commented-out examples, which are functionally identical) you have a pretty good example of an anti-pattern: you're directly mutating the nested properties of an object which is passed directly to attrs.

Mithril's reconciliation algorithm doesn't traverse the nested trees of the properties of attrs, since that would be a performance bottleneck. Strict equality of each property's previous/current values is used to determine whether a given element should be redrawn, which is why you see the result here. The direct mutation of a higher-scoped object is most at fault, which is illustrated rather succinctly by your working solution.

That being said, I think you have a pretty good candidate for a docs PR. I wouldn't mind seeing this explained in the anti-patterns sections of the vnodes page and the "m" api page, as well as in the final section of the autoredraw page.

@panoply
Copy link

panoply commented Apr 2, 2023

We do need more information pertaining to anti-patterns. IIRC @barneycarroll has written at length about this (but I might be mistaken). The example @gjmcn gives here is actually something I've seen many folks come across over the years.

@sonkwl

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Question For issues that are purely questions about Mithril, not necessarily bug reports or suggestions
Projects
None yet
Development

No branches or pull requests

4 participants