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

Modularize core functionality, consolidate global state #2

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

martinklepsch
Copy link
Owner

@martinklepsch martinklepsch commented Dec 19, 2016

Goals (tl;dr)

  • Modularize core functionality
  • Consolidate global state from non-core namespaces into core
  • Do all this while maintaining backwards compatibility

Goals (prose)

This is part of an attempt to consolidate state into re-frame.core so that other re-frame namespaces can be used in a more library-like manner. The goal is not to make this easy to use for regular re-frame users or even the default but rather step by step reduce how state is spread out across the codebase. Later on this should simplify the implementation of "multiple re-frame instances" and similar scenarios (e.g. devcards or using re-frame's eventloop/fx/cofx stuff with non-reagent projects).

There are a few critical pieces of state as far as I can see:

  • re-frame.registrar/registry for cofx event fx and sub
  • re-frame.router/event-queue FSM for processing events
  • re-frame.subs/query->reaction subscription cache
  • re-frame.db/app-db

Changes

  • The change done here moves the state from the re-frame.registry, re-frame.router, and re-frame.subs namespaces into re-frame.core and parameterizes the public API accordingly. Furthermore it turns the registry and subscription cache into a protocol and record similar to what's done in re-frame.router.
  • re-frame.db/app-db is still used directly as before.
  • re-frame.registrar has been renamed to re-frame.registry (why?)
  • Tests are passing but I'm not sure if the test suite is particularly expansive in the aspects affected by this change.
  • Example apps continue to work without modificaton

TBD

  • Moving the subscriptions cache into the frame singleton is still TBD
  • also the default interceptors used for reg-event-* should be provided as arguments when creating frames so that it is possible to create a frame without any interceptors.
  • there's some state in the tracing namespace that should potentially also become a part of a frame

[re-frame.registry :as reg]
[re-frame.std-interceptors :as stdi]))

(defprotocol IFrame

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is a good idea or not, but I wonder what would happen if we had IDispatch, ISubscribe, e.t.c.. Would that open things up for testing more, or is it unnecessary?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly thought about it but it didn't "jump at me" as the right way to do it. Also wanted to keep the change focused and free from smaller design decisions like this.

@danielcompton
Copy link

This is looking positive. I think the thing that Mike and I have always wondered is how this looks/works in a real application. Would you be able to port the todomvc to pass a frame around to see how it looks?

@martinklepsch
Copy link
Owner Author

martinklepsch commented Dec 19, 2016

@danielcompton The todomvc continues to work with these changes as the core API is maintained through the re-frame.core/the-frame singleton.

Mike mentioned that he is eager to see how this passing around would work and that he'd like to hold off changes like this one before that part is worked out...

The change presented here however isn't aiming to address that. Reagent doesn't provide a way to work with React's Context yet and I don't have any good ideas right now (besides childContext) for that part.

That said this change should make it a lot easier for everyone to experiment with their ideas around this problem which to me seems like a logical first step.

@danielcompton
Copy link

Ah of course, I see now.

@arichiardi
Copy link

arichiardi commented Dec 19, 2016

This honestly looks like a good ol' refactoring, with no API changes, which is good. I have not go deep into the code but I'd like to try it in one of my apps and report back.

@martinklepsch martinklepsch changed the base branch from master to frameless December 20, 2016 06:47
@martinklepsch martinklepsch changed the base branch from frameless to master December 20, 2016 06:47
@martinklepsch martinklepsch changed the title Turn core API into protocol and implement re-frame.core using a singleton Modularize core functionality, consolidate global state Dec 20, 2016
@martinklepsch
Copy link
Owner Author

@danielcompton @arichiardi @mike-thompson-day8

I updated this PR against latest master and implemented a few more changes (notably around the subscription cache and default interceptors).

At this point I would appreciate feedback & suggestions to carry this further. Also just running one or two more complex app on this branch would be of great help to find bugs the tests didn't catch. If that happens I would be happy to extend the test suite as well.

:event-queue event-queue
:app-db re-frame.db/app-db
:subs-cache subs-cache
:default-interceptors [(cofx/inject-cofx registry :db) (fx/do-fx registry)]})))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants