No changes detection using a reference
albertosantini opened this issue · 25 comments
I have been playing with the component example trying to separate the concerns in terms of model, view and controller.
Below a part of the state is created in a service and the reference is passed to the Introspected object.
Later the state is updated, but the Introspected callback is not triggered.
fooService.getItems() returns four items, foo.state.items contains the initial three items.
Have I been missing anything about Proxy?
class FooService {
constructor() {
this.items = [
{ id: 1, text: "foo" },
{ id: 2, text: "bar" },
{ id: 3, text: "baz" }
];
setTimeout(() => { // simulating an async update of the model
this.items.push({ id: 4, text: "minions" });
}, 3000);
}
getItems() {
return this.items;
}
}
class FooController {
constructor(root, fooService) {
this.render = hyperHTML.bind(root);
this.state = Introspected(
{
items: fooService.getItems()
}, state => FooTemplate.update(
this.render,
state,
events => this.handleEvent(events)
)
);
this.state.selectedItem = this.state.items[0];
}
...
The complete example: https://plnkr.co/edit/EZ8sMc7uJMgKmsvg0D8k?p=preview
it's not going to work like that.
Introspected imports objects and Array, it doesn't mutate anything.
If you change through the controller state you'll have any trigger you want.
If you just pass objects externally, these won't be changed or modified at all so that push in the Service will be just an array push, no side effects.
This is meant behavior, states should never be mutated from non owner.
TL;DR this is the key
this.state = Introspected(...)everything you do through the introspected state will trigger.
Anything else won't ever be noticed at all.
Anything imported won't be mutated or changed at all.
this.state or any variable assigned through Introspected is a whole new Proxied world.
You cannot make an object a proxy of itself by reference, so not only it's good because there are no side effects, it's inevitable because it's a technical limitation.
I hope it's clear. If not, ask more. Yet, not a bug 😉
@albertosantini You can do this instead https://plnkr.co/edit/IKZAtCemJimkavwc4s00?p=preview
If you want to have update events coming from outside, then you should probably subscribe to a observable or any event-source / emitter, and have the subscriber / observer trigger changes in the proxied state.
yep, passing through that state will work.
Also you shuold be able to pass just this instead of events => this.handleEvent(events) since whenever an event is an object with an handleEvent method it will be assigned as such like any DOM Level 3 event.
document.body.addEventListener(
'click',
{handleEvent(e) { console.log(e); }}
);😉
@WebReflection thanks for the details:
You cannot make an object a proxy of itself by reference, so not only it's good because there are no side effects, it's inevitable because it's a technical limitation.
I suppose "no-side-effects" is a strong selling point.
I was trying to replicate the angular(js)ish approach where the service is used as source of truth and the changes are automagically propagated using the js reference trick and, behind the scenes, the implementation of the observer pattern for the so called scope (an automatic $watch on it).
This kind of propagation (meaning AngularJS one) is a cons, because the state of the app is modified by the flow of the changes wihout any "control"; it works with small apps, but when the app becomes larger, it is a mess: this is the main reason of state management libs, like Redux, or for different approaches like in Angular or React.
Nice "trick" about the handleEvent, but I preferred mainly ${events} instead of ${this} in the template for readability and for creating an agnostic template based only on render, state and events objects.
@msand thanks for the suggestion to tweak the state directly in the controller: that was the starting point, but my aim was moving the changes in other parts of the code, but it seems I need to use a different paradigm due to @WebReflection's remark above.
Indeed using rxjs may be the definitive solution (but I am trying a custom and light approach), when there is not a user action updating the state (imagine stock quotes in a portfolio); otherwise, in this case, we can use your suggestion to modify the state in the controller.
For the sake of completeness I copy the complete example below.
Just a final note: my main aim of this working in progress snippet is creating a code style guide supporting an app with dozens of components and helping the migration from AngularJS to hyperHTML and Introspected.
Live demo: https://plnkr.co/edit/7AjKnVMdfAEC5UXwHFey?p=preview
"use strict";
// foo.template.js - it is the external template, replacing the html file
class FooTemplate {
static update(render, state, events) {
render`
<select onchange="${events}">${
state.items.map(item => hyperHTML.wire(item, ":option")`
<option value="${item.id}">
${item.text}
</option>
`)}</select>
<p>Selected: ${state.selectedItem.text}</p>
<ul>${state.items.map(item => hyperHTML.wire(item, ":li")`
<li> ${item.id} / ${item.text} </li>
`)}</ul>
<button type="button" onclick="${events}">Refresh the list</button>
`;
}
}
// foo.service.js
class FooService {
constructor() {
this.items = [
{ id: 1, text: "foo" },
{ id: 2, text: "bar" },
{ id: 3, text: "baz" }
];
}
getItems() {
return this.items;
}
refresh() {
return new Promise(resolve => {
setTimeout(() => { // simulating an async update of the model
this.items.push({ id: 4, text: "minions" });
resolve(this.items);
}, 2000);
});
}
}
// foo.controller.js
class FooController {
constructor(render, template, providers) {
this.fooService = providers.fooService;
this.state = Introspected({},
state => template.update(
render,
state,
events => this.handleEvent(events)
)
);
this.state.items = this.fooService.getItems();
this.state.selectedItem = this.state.items[0];
}
handleEvent(e) {
switch (e.type) {
case "change": this.onChange(e);
break;
case "click": this.onClick(e);
break;
default: console.log(e.type, "not implemented");
break;
}
}
onChange(e) {
this.state.selectedItem = this.state.items.find(
item => +e.target.value === item.id
);
}
onClick() {
this.fooService.refresh().then(items => {
this.state.items = items;
});
}
}
// foo.component.js
class FooComponent {
constructor() {
const el = document.querySelector("foo");
const render = hyperHTML.bind(el);
const fooService = new FooService();
new FooController(render, FooTemplate, {
fooService
});
}
}
// foo.module.js
new FooComponent();
I preferred mainly
${events}instead of${this}
sure thing. My hint was on what you pass, not what you call it as parameter 😉
Keep it as events in the static method, but pass this instead of events => this.handleEvent(events) otherwise you do what hyperHTML would've done for you.
On top of that:
handleEvent(e) {
const type = e.type;
// if you want to keep the onThingNamingConvention
const method = 'on' + type[0].toUpperCase() + type.slice(1);
//otherwise just 'on' + e.type;
return method in this ?
this[method](e) :
console.warn(type, 'not implemented');
}This would make classes easier to extend and it would give you a consistent pattern for events.
Ops... :) Roger and Copied.
class FooController {
constructor(render, template, providers) {
this.fooService = providers.fooService;
this.state = Introspected({},
state => template.update(
render,
state,
this
)
);
this.state.items = this.fooService.getItems();
this.state.selectedItem = this.state.items[0];
}
handleEvent(e) {
const method = `on${e.type}`;
return method in this ? this[method](e)
: console.warn(e.type, "not implemented");
}
onchange(e) {
this.state.selectedItem = this.state.items.find(
item => +e.target.value === item.id
);
}
...
@msand I investigated further the Emitter approach.
I was very tempted to use a micro lib for the Emitter part (there is one with no deps and 20-lines long, but not maintained), but I remembered there is a functional equivalence between the Emitter approach and Promise one: I have an AngularJS plunker showing that, just in case you are interested.
I said "functional" because the example only shows the results are the same, but it doesn't show any winner or doesn't resolve any cons for each approach.
So changing accordingly the code above using a Promise approach, when data is updated, for instance in the onmessage method of a WebSocket (receiving the ticks of the stock quotes), instead of calling the service, propagating the changes emitting an event, I would call a refresh method of the component, propagating the changes towards the service.
The refresh call of the component updates the view and its state; the refresh call of the service finally updates its internal state, just in case the service is called directly due to a user interaction.
It was enough to change the verse of the propagation, from bottom-up to top-down, to accomplish the mission. :) Anyway I am sure there are many solutions depending on the use cases and contexts.
@WebReflection Reviewing the code, I see a minor detail: passing this reveals all the class in the template, not only handleEvent.
Anyway there is a another point.
Generally speaking, this line this.state.items = items; is wrong.
It seems ok, the app works, but there is a subtle point behind the scenes. :)
I have been replacing the items of the state with new items: this means the whole list is rendered again, instead of rendering only the different items, because the references used in the wire are all new.
Depending on the goal, maybe replacing all the data is correct, we need to extend or merge the object (or array), like myUtil.extend(this.state.items, items).
My concern is myUtil.extend becomes a sort of giant object diffing utility, because we need to compare this.state.items and items and to act accordingly.
Does it make sense my concern?
passing
thisreveals all the class
If that's undesired, use the arrow function or this.handleEvent.bind(this) (but the arrow function is faster and shorter, for what is worth it)
I have been replacing the items of the state with new items: this means the whole list is rendered again
it depends. If new items come from an end point as unserialized items (e.g. JSON.parse) and you don't manually diff known items from unknown/new one, you'll have the same problem no matter what.
However, I see there's room for improvements in this area, the improvement would be in introspected 'cause known proxied objects should indeed never need to be recreated.
In some case I do the check but I don't in some other, I'll try to figure out when/where/how I'm failing at recycling what's there already.
I didn't want to abuse WeakMap but it looks like it might be inevitable in this case 'cause benefits are more than memory pressure issue.
I'll try to figure something out soon but for sure I don't want to deal with any known diffing otherwise I'll end up reinventing everybody-else wheel here.
Thanks
FYI after investigating about this issue I've decided to do a massive refactoring of the whole library.
The goal is to use one WeakMap per target instead of 2+, avoid double import when observed (since it is actually the most common case), bypass Proxy creation for the same object with the same observer.
In all this I'll probably lose the ability to import the same object data in many places without having a single conflict, but I guess for serializable data it's easy to deep-clone and pass new copies around, whenever it's needed (or simply use Object.create(data)).
I will come back on this, but not too soon (busy time these days)
@WebReflection Cool! Take your time. :)
I prepared an example showing how the state is updated and results in the dom with different kind of objects:
- Updating a primitive string
- Updating a string in an object
- Replacing an object with a new one
- Merging an object with another
- Adding a property to an object
- Updating an element of an array
- Pushing a new element to an array
Hope that helps during the refactoring to tune the integration between Introspected and hyperHTML.wire due to the proxied nature of the state.
Live example: https://plnkr.co/edit/J3vp5InwaKoXQFkcZi5h?p=preview
@albertosantini I haven't tested your demo 'cause I wasn't sure I've got them entirely.
I'll let you try Introspected version 0.2.0 which I've just published and work from there if anything breaks.
Fingers crossed, it shouldn't !!! 🎉
P.S. changelog here 5dccf29
Just to archive this issue, I investigated two scenarios: a simple hyperHTML snippet using an Introspected state and another snippet using a native (without Introspected) state.
Then I updated the state following the use cases listed above and using, or not using, merging techniques, rendering the results with the usual .wire approach.
So far it works fine and it makes sense.
I noticed only one difference between the Introspected state and the native one: when a new element is added to an Introspected array, the whole list is rendered; in the native scenario, only the new element is rendered.
that is covered by tests here and it should not happen.
Any example on how you change the array?
This works as expected and it should be similar to the tests:
render`
<ul>${state.items.map(item => hyperHTML.wire(item, ":li")`
<li> ${item.value} </li>
`)}</ul>
`;
...
this.state = Introspected({
items: [
{ value: "data[0] original value" },
{ value: "data[1] original value" }
]
}, state => template.update(render, state));
setTimeout(() => {
this.state.items.push({ value: "data[2] added" });
}, 5000);
This simplified snippet (leaving out services and the other details) reproduces the use case, that's the whole list is rendered:
const myData = [
{ value: "myData[0] original value" },
{ value: "myData[1] original value" }
];
this.state = Introspected({
items: myData
}, state => template.update(render, state));
setTimeout(() => {
myData.push({ value: "data[2] added" });
this.state.items = myData;
}, 5000);
Compared to this snippet, not using Introspected, similar to the previous in terms of updates and references, but only the new element is rendered:
const myData = [
{ value: "myData[0] original value" },
{ value: "myData[1] original value" }
];
this.state = {
items: myData
};
template.update(render, this.state);
setTimeout(() => {
myData.push({ value: "data[2] added" });
this.state.items = myData;
template.update(render, this.state);
}, 5000);
I think you are forcing a non real-world use case.
You either use introspected as state, or you don't.
I cannot do much with external data because like I've said I don't touch it, and I don't want to use it right away: I am trying to avoid side effects.
If you keep passing external objects, introspected will keep importing them, as easy as that sounds.
I'm afraid this is really a won't fix, one should not mix up introspected data and its original external source.
Hope it's clear why that happens.
P.S. to give you a better idea, import the same data in two different Intrsopected objects and see that changing one would not affect the other and vice-versa.
This is meant, and it's a feature, not a bug. Imported data is fresh new every time.
Just don't mix up two different sources of truth.
P.S. 2 see here how to eventually diff the original source and introspected data:
https://github.com/WebReflection/introspected/blob/master/test/how-to-diff.js
Sorry to bother you: I think you explained it twice or three times. :)
Behind the scenes, in my mind, I have the following equivalence:
state + manual update === Introspected
Well highlighted in the last snippet.
The only difference is the native state object with the manual update calls w.r.t. the Introspected object.
I was following this rationale: there is one state (an Introspected object) per controller (or view) and there are different sources, usually plain old javascript objects (POJOs), mashing-up the state.
I think I need to review the rationale, accepting your suggestions about diffing or using Introspected objects (vs, POJOs) also outside the controller.
So far thanks a lot for the effort.
state + manual update === Introspected
Nope, this is wrong.
If your state is an introspected instance you do updates through it. That's it, forget about the external source. Indeed, you're better off never referencing it at all and all your problems will disappear.
Or better, your first example is everything you need.
Every other framework requires that you invoke a setState with new data you would never manually change.
var myData = {items: [1, 2, 3]};
this.setState({data: myData});
// who does this? this is bad!
myData.items.push(4);
// what do you expect now?Introspected is here to avoid the need to manually call setState and instantly reflect the state on the view (and vice versa).
You want to save the state? jsonState = JSON.stringify(this.state) and that's just about it.
You want to impot the state? this.state = Introspected(JSON.parse(jsonState));
You want to do this instead?
var state = JSON.parse(jsonState);
this.state = Introspected(state);
state.items.push(4);if that's waht you want to do, you simply don't need Introspected because you want to manually update 😉
Now I see my fault.
The Proxy approach creates a separation between the state and the underlying reference of the object (good for state isolation) and I should not reuse reference patterns for the changes propagation.
I think you are forcing a non real-world use case.
Agreed. I would like to mix the state with external sources like:
const myData = [
{ value: "myData[0] original value" },
{ value: "myData[1] original value" }
];
this.state = {
items: myData
};
this.dirtyCheck = Introspected({
counter: 0
}, () => template.update(render, this.state));
setTimeout(() => {
myData.push({ value: "data[2] added" });
this.dirtyCheck.counter += 1;
}, 5000);
The snippet above works as expected, rendering only the new element, but it is a fake, it is like to manually update.
Got it: import, apply changes to proxied state, update and loop. :)
That example is exactly like a setState. You don't need Introspected there.
const myData = [
{ value: "myData[0] original value" },
{ value: "myData[1] original value" }
];
this.state = {
items: myData
};
setTimeout(() => {
myData.push({ value: "data[2] added" });
template.update(render, this.state);
}, 5000);That's it.
You would use Introspected only to do the following:
this.state = Introspected({
items: [
{ value: "myData[0] original value" },
{ value: "myData[1] original value" }
]
}, () => template.update(render, this.state));
setTimeout(() => {
this.state.items.push({ value: "data[2] added" });
}, 5000);The point is that you can pass this.state.items around, if needed to be handled exeternally, and whenever that happens the view will change.
var myData = this.state.items;
setTimeout(() =>{
myData.push({another: 1});
// the update callback will trigger
}, 5000);That example is exactly like a setState. You don't need Introspected there.
That was my point. We are on the same page. :)
The point is that you can pass this.state.items around, if needed to be handled exeternally, and whenever that happens the view will change.
This is the opposite I am used to do and it is a great suggestion.
I think I have all the elements in the right order now... I hope. :)