canjs / canjs

Build CRUD apps in fewer lines of code.
https://canjs.com/
Other
1.91k stars 421 forks source link

v2.3x | %root on Component view model is always converted to Map, causing performance problems #2969

Open rjgotten opened 7 years ago

rjgotten commented 7 years ago

In v 2.3.x the initialization of can.Component instances adds a %root property that represents the root view scope to the view model's initial data.

When the root view scope is a plain JS object then when that data is turned into a view model instance, the can.Map logic will detect %root% as being capable of being upgraded into a can.Map and will do so.

This can result in a huge decrease in performance when %root contains a large object graph that is intentionally non-observed and developers have no way of preventing this outside of polluting their view model definitions with an additional define for %root which instructs to pass it through unmodified. That's assuming they're using the define plugin; are using a proper classed view model to begin with; and know that they can fix this problem that way.

To prevent this from happening, the fallback default can.Map.prototype.__type(value, prop) implementation should be overridden on view model can.Map subclasses to always pass value directly back when the prop name equals"%root".

gsmeets commented 7 years ago

can't you circumnavigate the problem by passing in a can.Map with your unobserved data as a defined property on it?

rjgotten commented 7 years ago

Yes, you can. But that is only another variation of having to tailor your model to work around the problem.

gsmeets commented 7 years ago

Well, in my personal opinion always enforcing and having the developer assume that the root model is a can.Map is fairly intuitive. The whole idea of not having something observed on purpose is an advanced use case. But that's just my 2 cts.

rjgotten commented 7 years ago

Well, in my personal opinion always enforcing and having the developer assume that the root model is a can.Map is fairly intuitive.

Except that's not the case. It's currently only the case for scopes created for a component. If you push a plain object into a stache template, %root will infact be just that plain object, as it's supposed to be.

A big problem here is that even if I have a Map subtype that uses { type : "*" } to define an unobserved plain object; if that plain object at one point touches a bit of rendering with another nested component, the remainder of the object graph will still be converted to a Map and be assigned as the %root of that subcomponent's view model.

It's infectious that way.

The only way to avoid that from happening from 'user land' is to create a special helper which takes the current plain object scope and creates a new Map scope with all known keys of the plain object set to { type : "*" } and to instantiate the subcomponent within that scope only. And quite frankly; that's INSANE.


Besides; if the original root scope is not a Map to begin with, then what is there to gain from converting the %root token to one?

Adding observability? Well; no. If the underlying data is not observable to begin with, wrapping it in a Map will do squat to make it observable.

Consistent API? No; because stache itself allows all kinds of scopes, from plain objects to Map instances to computes. And if you write a general purpose helper, you should be able to handle all that. (Note: this is why there's a shared resolve method used by some of the built-in helpers.)

justinbmeyer commented 7 years ago

@rjgotten I think what you are saying is reasonable. I'd accept a patch with this change and we can get out a new 2.3 release. (3.0 doesn't have %root anymore).

rjgotten commented 7 years ago

@justinbmeyer I'll see what I can do to make that happen.

gsmeets commented 7 years ago

@justinbmeyer what alternative do we have for %root in 3.0?

matthewp commented 7 years ago

@gsmeets can-route's .data property.

gsmeets commented 7 years ago

that's great for those of us that don't use can-route. :s

matthewp commented 7 years ago

Then just export your root VM as a module and import it wherever you need it.

gsmeets commented 7 years ago

My root viewmodels are the viewmodels of dynamically inserted components. (It's an SPA with a tabbed interface). Since it's not a singleton viewmodel I don't think that'll work.

rjgotten commented 7 years ago

@gsmeets While in CanJS 3.x the %root attribute is no longer present directly on Component viewmodels, Scope.prototype.read still recognizes %root as a special token, just as it did in CanJS 2.x, and still short-circuits it to the Scope.prototype.getRoot method.

Unless I'm missing something, that should still produce the same root object for you.

matthewp commented 7 years ago

@rjgotten Good info! Note that if you intend to use Scope.prototype.getRoot I would highly recommend filing an issue with https://github.com/canjs/can-view-scope/issues so that it becomes a public API and we don't remove it not knowing its being used.

gsmeets commented 7 years ago

I think I'm better off passing in a custom marker myself for those cases. I'm already using a custom component to "inherit" from. I can fold in that functionality in there I think.

It's really quirky that you can't easily reference the component's viewmodel in whose stache template you're writing your code. Any property that you need to bind on it you need to initialize to null for it to be found during template binding. In most cases I circumnavigate this with binding stuff to submodels on my viewmodel, but you can't always do that, unless you create a bunch of extra models for no apparent reason.