-
-
Notifications
You must be signed in to change notification settings - Fork 715
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
Make it okay to use subscribe
in Form-1 components
#218
Comments
subscribe
in Form-1 componentssubscribe
in Form-1 components
I'd be inclined to vote against this change, but am willing to be persuaded. First, lets talk about the benefits:
AFAICT those are the benefits to allowing/encouraging this behaviour. Now for the downsides:
A wise man said "the best gift you can give yourself is a good problem statement" 😄. I think we've started at the point of "should we support subscriptions in form-1 components", without examining the problem deeply enough. If the problem is that people find the distinction between the two confusing, then alternative idea would be for re-frame to warn the user (and give remediation instructions) when they create subscriptions outside of the React context of There is a definite learning curve to get over with Reagent form-1 and form-2 components, but I'm not sure if this is the right answer. Perhaps I haven't understood the problem myself though... |
I'm in favor of such a change. I agree with the benefits as outlined by @danielcompton. Essentially this change would make re-frame simpler and easier to use, which is a key goal of re-frame. Now let me answer to each of the downsides as pointed out by @danielcompton, in order:
All in all my opinion is that subscribe-in-Form-1 has the potential to make re-frame use significantly simpler than it is now. |
Unless I misunderstand @danielcompton's point was that subscribe-in-Form-1 will make re-frame more complex, not simpler. If so, I totally agree and don't think this change should be made. The macro on the wiki makes form-2 more convenient for users who want to opt in to something that looks like subscribe-in-Form-1 but it shouldn't be the default to hide how reagent/re-frame works in this regard; e.g., (defn- to-sub
[[binding sub]]
`[~binding (re-frame.core/subscribe ~sub)])
(defn- to-deref
[binding]
`[~binding (deref ~binding)])
(defmacro let-sub
[bindings & body]
`(let [~@(mapcat to-sub (partition 2 bindings))]
(fn []
(let [~@(mapcat to-deref (take-nth 2 bindings))]
~@body))))
(defn panel
[]
(let-sub [v [:some-sub]]
.... |
Moving from the Corner Case #1Consider this classic Form-2: (defn view
[x] ;; x is a value like 42
(let [sub (subscribe [:something x])] ;; <-- note use of x's value
(fn [x]
[:div str(@sub)]))) This view has a problem if To solve this problem, we created dynamic subscriptions - the 2-arity version of (defn view
[x] ;; x is a ratom
(let [sub (subscribe [:something] [x])] ;; reactive `x` supplied in 2nd vector
(fn [x]
[:div str(@sub)]))) I've never really liked the dynamic subscriptions solution. It means someone has to FIRST get an annoying paper cut and, only then, discover dynamic subscriptions to fix that paper cut. Nicer if it all just worked the first time. Nicer if the programmer's mental model was matched. If this view was written as a Form-1, it just works first time, no paper cut and no eventual discovery of dynamic subscriptions: (defn view
[x] ;; x is a value
(let [sub (subscribe [:something x])]
[:div str(@sub)])) When Corner Case #2Consider this slightly contrived code: (defn view
[]
(let [errors? (subscribe [:errors?])
error-list (subscribe [:err-list])]
(fn []
(if @errors?
[error-view @error-list]
[:div "No errors"))) Notice the Now, imagine further that, for the entire existence of a So, there would be a zombie subscription. (This virtually never happens in practice .. the examples have to be a bit contrived and poorly written). This is a corner case, and it is easily fixed if you just add an apparently meaningless Again, use of a Form-1 means this issue never ever even comes up. It just works. ResultI continue to be of a mind to provide this guarantee in v0.9.0. I see it brings a nice simplicity and removes corner cases. |
It now occurs to me that we can go one step further in this process to ALSO remove a source of annoying, newbie bugs (at the expense of adding to the re-frame API). Consider this code: (defn view
[x] ;; x is a value
(let [sub (subscribe [:something x])]
[:div (str sub)])) See the bug? Hint: it is in the last line. Further hint: something is missing. Yep, it should have been We could create a new API function called, say, (defn input-signal
[v]
(deref (subscribe v))) ;; @(subscribe v) In effect, this function does nothing other than to automatically deref the subscription. Forgetting to deref a subscription is such an annoying pebble in the shoe, and being able to never trip across that problem again is quite attractive. Use of this new function (instead of (defn view
[x] ;; x is a value like 42
(let [val (input-signal [:something x])] ;; <-- note use
[:div (str val)]))) ;; no @ needed on val The argument against this proposal: we'd then have two functions doing something similar, leading to possible confusion. I have a test which I use when assessing adding something: how many pages of documentation do I have to write to satisfactorily explain this, and that gives me pause for thought on adding this Naming Competition: if we did do this, what name should be used? |
|
I think that the underlying problem here is the reagent component api. On the other hand in reagent it was always possible to access external state, as form 1 components can assess globally defined ratoms. see the following code from the reagent tutorial
Would it be possible to move ALL subscriptions within a form 1 component. If so should this then become the default? Maybe that may be a better way to do things? |
@danielcompton some valid concerns about downsides, but I also acknowledge corner case #1 & #2 in follow up from @mikethompson I'm a little reserved about catering for newbie bugs via automatic deref via input-signal which imho also further hides the reagent/react comp details, nonetheless my competition submission |
competition entry is |
It is now officially guaranteed that you can use The issue of a further helper function is not yet decided. |
Does this also green light putting subscriptions in the computation function passed to |
@gpellis no, absolutely not. |
Seeing as this is still open: my 2p is that the original proposal solves the right problem wrongly. The problem is the reagent API and how Changing the behaviour of another lib's API always leads to confusion. Adding a new API to make the other lib's API easier is the way to go, and whilst I think the idea of a new API that doesn't require Just my 2p :-). |
This guarantee is officially part of v0.9, which is about to be released, so I'm going to close this (it also works in v0.8, but wasn't offical) For the moment, I can't figure out how to add a So, I'll let everyone add their own 4 line |
This saves us having to wrap the components on a on let/fn and makes things less nested. It's been the case since re-frame 0.9, but I hadn't gotten around to changing my components. See: https://lambdaisland.com/blog/11-02-2017-re-frame-form-1-subscriptions day8/re-frame#218 Also: - Moved the reminder load to the `:state-ui-section` handler, which is much cleaner. Shouldn't have unnecessary side effects on render. - Refactored a few functions/components for readability.
(defn bad-component
[foo?]
(if foo?
;; The mounted component will react only to the sub made in the inital render, and the other sub will be
;; born a zombie when made because Reagent isn't listening for derefs on re-render
@(rf/subscribe [:sub/bar])
@(rf/subscribe [:sub/baz])))
(defn another-bad-component
[x]
; when x changes after the component is mounted, the component is still stuck reacting to the old sub
@(rf/subscribe [:sub/foo x]))
I retract this. See #797. |
Should re-frame now allow
subscribe
within a renderer?In concrete terms, can this (current approach):
... to become this (proposed):
Notes:
I probably won't struggle too much to make this happen BUT if it naturally falls out of the current implementation it could be a good idea to officially guarantee the behavior. Just so everyone knows where they stand.
Someone needs to test if this works already ... does the new cache ensure that subscriptions are reused and not constantly created and destroyed on each rerender. Actually, in the time it has taken to write this ticket, I've gone from being 80% sure this will work to about 99% sure it will work. But a test needs to be done. And some more thinking about possible edge cases.
The text was updated successfully, but these errors were encountered: