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

View render call does not respect direct attributes object modification #2719

Closed
MrSimmmons opened this issue Jan 25, 2022 · 4 comments
Closed
Assignees
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage

Comments

@MrSimmmons
Copy link

Mithril version:
2.0.4

Browser and OS:
Chrome Version 97.0.4692.71 (Official Build) (64-bit)
Linux Ubuntu 20.04

Code

https://jsfiddle.net/dbo08vkf/2/

Expected Behavior

In the code example above, Input 2 should behave the same as Input 1 when their corresponding "toggle disabled" button is pressed

Current Behavior

When inputs (and im guess all html attributes) are supplied with an direct object instead of a fresh object with individual attributes, when one of the attributes gets updated, the render process does not respect the updated value

Steps to Reproduce

Supply a direct attributes object to the m() function instead of building a new one in the view call.

Context

(FYI I'm using TypeScript for my project, but this issue can still be observed in the example above) I'm trying to build a light wrapper around the native HTML components, while also giving the ability to make custom components with more complex use cases. And in the terms of the native HTML components, it is a lot easier to just supply an object it their constructor which is made up of their already existing attribute options instead of having to have private class values for each possible input type, then having to rebuild that object every time on render.
This is very much a code quality issue rather than a performance issue. Because it will allow me to do this

view: () => m('input', this.options)

instead of this

view: () => m('input, {
  disabled: this.disabled,
  value: this.value,
  onclick: this.onclick,
  oninput: this.oninput,
  onchange: this.onchange,
  // etc etc...
})

for every native wrapper

@MrSimmmons MrSimmmons added the Type: Bug For bugs and any other unexpected breakage label Jan 25, 2022
@StephanHoyer
Copy link
Member

StephanHoyer commented Jan 29, 2022

minimal repro on next branch

Seems to work on other attributes

@StephanHoyer StephanHoyer added the Area: Core For anything dealing with Mithril core itself label Jan 29, 2022
@StephanHoyer
Copy link
Member

The reason for this is, that when you mutate attrs you also mutate the internal reference to attrs (which is the same object) and therefore disabling the diffing here. So mithril can't detect this change.

It only works on value since value is handled differently.

We should probably extend this part of the doc to attrs-mutability
https://mithril.js.org/vnodes.html#avoid-memoizing-mutable-vnodes

Maybe we should even warn about not using identical attrs-object of a vnode-rerender

@StephanHoyer
Copy link
Member

@pygy @isiahmeadows we can do something like

 	function updateAttrs(vnode, old, attrs, ns) {
+		if (old && old === attrs) {
+			throw new Error("Don't reuse attrs object, use new object for every redraw")
+		}
 		if (attrs != null) {
 			// If you assign an input type that is not supported by IE 11 with an assignment expression, an error will occur.

in render/render.js but that might have negative performance implications, right?

@MrSimmmons
Copy link
Author

minimal repro on next branch

Seems to work on other attributes

Ahhhhh I didn't think of using spread here. And yeah having a warning (and maybe even a suggestion to use spread in this case) is a good idea.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Projects
Status: Closed
Development

No branches or pull requests

3 participants