athensresearch / athens

Athens is no longer maintainted. Athens was an open-source, collaborative knowledge graph, backed by YC W21
https://athensresearch.github.io/athens
Other
6.32k stars 397 forks source link

Migrate from re-frame to reagent #95

Closed tangjeff0 closed 4 years ago

tangjeff0 commented 4 years ago

06-07-2020 Update

daveduthie commented 4 years ago

@tangjeff0 is this mostly about devcards? Or are there other reasons too?

If it's the former, have you considered a dependency-injection approach to making components devcards-friendly? This is a pattern we've used quite succesfully in a component library I maintain. For example:

[some-comp {:model        *some-document
            :subscribe-fn could-be-reframe-subscribe
            :dispatch-fn  could-be-re-frame-dispatch
            :more         :args}

The component takes a :model, which could be a reagent atom or a re-frame subscription. Components read upstream changes via model and send changes downstream via dispatch-fn or, if that's not present, fall back to swap!-ing on model. Richer interactions with the environment all go through subscribe-fn and dispatch-fn.

tangjeff0 commented 4 years ago

@daveduthie part of this is definitely DevCards, and more generally, component isolation. I wasn't aware of dependency-injection in this case, but that seems like a very interesting path! Curious to hear others' opinions @jeroenvandijk @tomisme .

jeroenvandijk commented 4 years ago

The reason I don't think re-frame is suitable is because of the lack of being able to do dependency injection properly. If we could pass the app-db as an argument to subscribe etc this would be a whole lot easier.

I think the proposal of @daveduthie, with all respect, is a start but not as good as I would like to have it. I'm used to making a whole application deterministic by having deterministic components in tests (mocking time, randomness etc). This what I would like to see with Athens as well.

tangjeff0 commented 4 years ago

As long as there is a need for global state or actions, there are still use cases for re-frame. In particular, routing should still be handled with re-frame.

But for datascript, we don't need to use re-posh by any means. Querying or transacting to datascript is already global. Creating ~re-frame~ re-posh subs or events is not useful, AFAICT. Query API is just Datalog, Pull API + Transactions are just maps and vectors. We can compose and reuse these independent from ~re-frame~ re-posh patterns.

avichalp commented 4 years ago

As long as there is a need for global state or actions, there are still use cases for re-frame. In particular, routing should still be handled with re-frame.

This is a good point.

Re-frame's app-db can be used to store state that does not belong in datascript like state that describes UI. Routing is a good example. Another example could be: imagine, based on a user's preference, a component that can be rendered as a list or a grid using the same (datascript) data. This user preference can be stored in re-frame's app-db, and the component can subscribe to it.

I think it makes more sense to use both re-frame db (with subscriptions) and datascript (without re-posh) instead of completely removing re-frame.

tomisme commented 4 years ago

Re-frame's app-db can be used to store state that does not belong in datascript

Hmm I'm not convinced of this. Isn't re-frame's app-db essentially just another database, like datascript?

The global-ness of re-frame has been frustrating in projects I've worked on in the past, like @jeroenvandijk says, it's not deterministic. It's a trade-off though (see the re-frame docs), the global registration funcs are easy to use and end up just wrapping a lot of lovely pure functions you write as an app developer.

re-frame includes some useful functionality that we'll need a solution for (e.g. event loop, interceptors) and some that we don't (e.g. subscriptions). Could we start by using re-frame for the parts we need but with a layer of indirection on top? That way we could gradually swap parts out as we develop our own solutions. re-frame isn't magic, but there's a lot of person-hours and knowledge embedded in it. The beauty of open source is we can use the parts we like!

Long term, I'd love to see us pull our solution out into a re-frame-like library built on top of datoms/EQL, that's a little more pluggable.

Ultimately, my dream is to see 'Athens-in-Athens', which re-frame makes harder :smile:

avichalp commented 4 years ago

Isn't re-frame's app-db essentially just another database, like datascript?

@tomisme: Sure, they are essentially same. What I meant was: when we need to use subscriptions over a global store, we can use re-frame's app-db instead of datascript+re-posh.

I don't have many frustrations with global-ness or non-determinism of re-frame but now that you have pointed out:

global registration funcs are easy to use and end up just wrapping a lot of lovely pure functions you write as an app developer

I am also leaning towards using reagent components with local states. Is that what you are suggesting?

Could we start by using re-frame for the parts we need but with a layer of indirection on top?

I agree with the general direction here. As you already pointed out we need a re-frame like library built on top of datums but building that up from scratch will be a huge task. However, I am not clear yet how this indirection layer would look like. It would be interesting to explore what kind of API this indirection layer provides 🤔

Also, I like the idea of "Athens-in-Athens" 👍
Perhaps this can even make it easy to put Athens on multiple platforms in the future like mobile, desktop etc.

tangjeff0 commented 4 years ago

re-frame includes some useful functionality that we'll need a solution for (e.g. event loop, interceptors) and some that we don't (e.g. subscriptions).

Yup, I'm now passing dbs to components that need datoms — Athena, left sidebar, All Pages table. This makes DevCards a lot easier to work with too.


We don't need re-posh. I'm not entirely sure what posh does either because some of our components use posh/query, while some use d/query, and I haven't noticed a difference.


Some use cases I can see for re-frame:

Any others?


Finally, would love to use EQL long-term! Especially when we are joining multiple data sources (and therefore ontologies) 😄 @avichalp @tomisme

tomisme commented 4 years ago

I think that without posh our reagent components won't re-render when the db changes, it's posh that's watching the datascript transaction log.

tangjeff0 commented 4 years ago

@tomisme yeah apparently posh makes components work with reagent. But Athena and db_boxes use vanilla datascript and they seem to update reactively 😆

tangjeff0 commented 4 years ago

I think I see the difference. I don't think Athena, db_boxes, or all_pages actually update reactively. datascript queries do not return Reagent atoms. But none of these use cases actually need to be reactive. They are just returning data.

I realized this because I'm doing pages and linked references right now. In this case, you do want blocks to be reactive because changing a block could change an embed or transclusion on the same page.

But one lame thing about posh is that it does not have pull within q. That means we cannot do:

(d/q '[:find [(pull ?block [:db/id :block/uid :block/string :node/title {:block/_children ...}]) ...]
    :in $ ?query-pattern
    :where
    [?block :block/string ?txt]
    [(re-find ?query-pattern ?txt)]]
  db
  (re-case-insensitive query))

Source: https://github.com/mpdairy/posh/issues/21

tangjeff0 commented 4 years ago

Doing global (window) event listeners now. re-frame has been extremely useful in this area. Will link to PR when up.

In the mean time, #156 gets rid of re-posh. Now just using a single global posh connection.

tangjeff0 commented 4 years ago

This commit introduces some subs events and dispatches that make blocks more interactive: https://github.com/athensresearch/athens/commit/4a40ecd17aee33bf5cc8027c880f39ac3a40d564#diff-76388d867ca290c8ab63497fefeb16f1R168-R179

Closing this issue for now as the value prop of re-frame has basically been proven for our use cases.