levand / quiescent

A lightweight ClojureScript abstraction over ReactJS
Eclipse Public License 1.0
613 stars 46 forks source link

Idea: Implement shouldComponentUpdate using a protocol (DataScript compatibility) #35

Closed Peeja closed 9 years ago

Peeja commented 9 years ago

Quiescent has trouble handling DataScript databases as values, because checking their equality is non-trivial, and forces a bunch of normally-lazy evaluation. But Quiescent doesn't need to know for certain that the value has changed, it just needs to know if the value might have changed.

I'd like to have shouldComponentUpdate use, instead of not=, a protocol function which tells us whether the value is definitely the same or possibly different, in some efficient way. The default implementation would use not=; DataScript DBs could override that to check, say, whether the new DB has seen any new transactions since the old one. If it has, the values inside might still be the same if we check them, but it's faster to say they might be different and let the component re-render.

I'm happy to do the work and PR it, but I wanted to get your thoughts on the idea first.

(This conversation that gave me the idea.)

ghost commented 9 years ago

Improving datascript compatibility would be really cool in my opinion.

frankiesardo commented 9 years ago

You can already adhere to shouldComponentUpdate by generating an appropriate data structure that is passed to the components (as opposed to the entire DB).

IMO this is something that you'd want to do anyway for non-trivial apps: a change in the model doesn't mean the data that is used by the particular view needs to update (think about multi-pane SPA).

ghost commented 9 years ago

@frankiesardo Yeah but that is a fundamentaly different style of architecture. When generating an intermediary one could also use Om with its opinionated data model. I think the better abstraction would be the UI as a form of database view (view in the classical database sense) where updates are calculated incrementally and efficiently. (No need to crawl the entire database to generate a new DS when the database can calculate a diff for the UI during a transaction.)

frankiesardo commented 9 years ago

You'd be crawling the database anyway while fetching the information you need. But instead of hiding the queries in each component you explicitly state which values each component needs, which in turns improves code readability. Think of it like "templating". Moreover, while debugging it's much easier to inspect a minimal data structure tailored for the current view rather than entire database.

Depending on the entire database doesn't scale anyway: if you have more than one "page", with different models you would keep updating the current view even when new information that is not needed comes in. The power of Datascript is path-independent updates, which frees you from Om. But generating a tree-like structure while rendering is still a good thing for fast diffing. Mind, the tree is not the model: it's just a way to convey values where needed.

ghost commented 9 years ago

Most paper call the diff generation "incremental view maintainance", the Idea is that you take the queries that define a view and rewrite them in such a way that they are of the form ((old-qresult U (q' old-db delta-added)) / (q' old-db delta-removed)) Which means that if you have, say, a join operation it will be bounded by |tx| * |db| instead of |db|^2, which will be significantly smaller with a high probability.

If you make rendering dependent on the change of these views. You neither crawl the entire database, nor do you have to update when something changes that does not change the query.

levand commented 9 years ago

This is an interesting idea.

I'm not inherently opposed to it, (DataScript support would be awesome) but I want to put at least a tiny bit of thought into other ways we could get that. Protocols are awesome, but invoking the "default" implementation of a protocol can sometimes be surprisingly slow (as it has to eliminate all other possible paths).

One other option would be to simply write your own version of quiescent/component. The function is really straightforward, and writing a version that tested if the data was a DataScript value should be simple. I agree that "out-of-the-box" support would be nice, but given that it's not hard to work around I hesitate to add a performance penalty (however small) to all users.

Peeja commented 9 years ago

Ah, I didn't realize invoking the default implementation was so slow. That's probably a dealbreaker, then.

I do think there's a use case in here somewhere for a custom shouldComponentUpdate. As @frankiesardo says, depending on the entire DB can be an antipattern, but I think there's probably a viable strategy in defining shouldComponentUpdate in terms of your data model, so you don't updated your views on unrelated DB updates.

But that's not worth impacting the performance of normal quiescent/components. Maybe it's worth having a variant which uses a protocol? Maybe that should be a different library altogether? In any case, it's not worth writing until it's written for an application, and I no longer have a need for it at the moment, so I'm happy to let this go.

I'll close this issue. If anyone feels like running with the idea in a new issue/PR/repo, I'd be interested in taking a look.