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

Reagent 0.5.0-alpha #76

Closed
holmsand opened this issue Dec 10, 2014 · 58 comments
Closed

Reagent 0.5.0-alpha #76

holmsand opened this issue Dec 10, 2014 · 58 comments

Comments

@holmsand
Copy link
Contributor

I've finally pushed a 0.5.0-alpha version to clojars, with support for React 0.12.1 and much more.

Read all about it here:

http://reagent-project.github.io/news/news050.html

Take a look, test, and tell me what you think!

I've also added some support for publishing to the doc site directly from the Reagent repo: it is now as simple as make push-project-docs (so the doc site now runs 0.5.0-alpha).

@ptaoussanis
Copy link
Contributor

Fantastic Dan, thank you! Very happy to see momentum picking up for Reagent. Cheers! :-)

@mike-thompson-day8
Copy link
Member

This progress has made my day. Many Thanks.

But I'm puzzled. I can't see the benefit in wrap. I rewrote your example without wrap, and it reads just fine, I think.

https://gist.github.com/mike-thompson-day8/9594193b19c03d5cfda7

I feel like I'm missing something big here.

EDIT: okay the penny has dropped. Its the way that wrap can be, um, hierarchically applied to step "down" through the levels of a data structure, much like a cursor, but still providing a way for the child components to do swap! and reset!.

@holmsand
Copy link
Contributor Author

There's nothing really big about it really... You got exactly the point: it is just a value + callback, and it works (almost) exactly as if you passed them separately. It is a very simple abstraction.

But it does, I think, make intent a little clearer in code, and makes it easier to handle hierarchies without going crazy in the process.

It also makes it easier to write performant code – if you're going with callbacks, you'll probably use anonymous functions, and since they will be different every time the parent renders you'll end up re-rendering all the children as well.

A couple of other benefits: wrap should make it trivial to go from using local state to (part of) global state passed from a parent. And since the result from wrap sort of feels like a cursor, it ought to make cursor-loving people a little happier, without having to give up total control over data-flow.

@luposlip
Copy link

w00t! :-D

On 10 December 2014 at 13:05, Dan Holmsand notifications@github.com wrote:

I've finally pushed a 0.5.0-alpha version to clojars, with support for
React 0.12.1 and much more.

Read all about it here:

http://reagent-project.github.io/news/news050.html

Take a look, test, and tell me what you think!

I've also added some support for publishing to the doc site directly from
the Reagent repo: it is now as simple as make push-project-docs (so the
doc site now runs 0.5.0-alpha).


Reply to this email directly or view it on GitHub
#76.

@seancorfield
Copy link
Member

For comparison, here's the cursor version of that example (forked and updated from @mike-thompson-day8 's gist and completely untested):

https://gist.github.com/seancorfield/8f62615233244820c8db

@holmsand
Copy link
Contributor Author

Yes, that looks about right. It should be fairly painless to switch from one to the other.

There's one difference to be aware of, though: the cursor version will scale much worse, since the version of cursor in the library re-renders every component that depends on a given atom whenever it changes, even if the change doesn't change a given cursor's view of the atom (and, as I said above, you lose control over data flow).

@seancorfield
Copy link
Member

Right... to make cursor as efficient it would have to be in core and the wiring would need to be changed. Given folks don't seem to want cursor in core, that's the price that gets paid :)

@seancorfield
Copy link
Member

Data point: we just upgraded to 0.5.0-alpha and everything seems to work just fine (after we removed our dependency on [com.facebook/react "0.11.2"] and updated our :preamble!).

@dmohs
Copy link

dmohs commented Jan 17, 2015

I have a working project in 0.4.3. Changing only the Reagent version to 0.5.0-alpha results in these warnings on a lein cljsbuild auto dev in a newly created directory:

WARNING: Use of undeclared Var reagent.impl.util/some? at line 123 file:/Users/dmohs/.m2/repository/reagent/reagent/0.5.0-alpha/reagent-0.5.0-alpha.jar!/reagent/impl/util.cljs
WARNING: Use of undeclared Var reagent.impl.template/some? at line 61 file:/Users/dmohs/.m2/repository/reagent/reagent/0.5.0-alpha/reagent-0.5.0-alpha.jar!/reagent/impl/template.cljs
WARNING: Use of undeclared Var reagent.impl.template/some? at line 62 file:/Users/dmohs/.m2/repository/reagent/reagent/0.5.0-alpha/reagent-0.5.0-alpha.jar!/reagent/impl/template.cljs
WARNING: Use of undeclared Var reagent.impl.template/some? at line 64 file:/Users/dmohs/.m2/repository/reagent/reagent/0.5.0-alpha/reagent-0.5.0-alpha.jar!/reagent/impl/template.cljs
WARNING: Use of undeclared Var reagent.impl.template/some? at line 77 file:/Users/dmohs/.m2/repository/reagent/reagent/0.5.0-alpha/reagent-0.5.0-alpha.jar!/reagent/impl/template.cljs

(lots more of the same follow)
The project's only dependencies are:

[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2202"]

cljsbuild is 1.0.3. Any debugging help would be appreciated.

@seancorfield
Copy link
Member

@dmohs did you do lein cljsbuild clean first?

@dmohs
Copy link

dmohs commented Jan 17, 2015

@seancorfield I did. Also, my build script deletes the build directory before starting, so my builds are always clean, and cljsbuild auto takes care of making sure development iterates quickly.

@mdhaney
Copy link

mdhaney commented Jan 17, 2015

@dmohs Need to update Clojurescript to at least 2261. The older versions don't appear to have 'some?' implemented.

Edited - had wrong version in initial reply.

@yogthos
Copy link
Member

yogthos commented Jan 20, 2015

So, I've been using the alpha for the past month in my apps and I've seen no issues. Might it make sense to push out RC at this point or are there any major outstanding issues anybody noticed?

@dmohs
Copy link

dmohs commented Jan 21, 2015

@mdhaney Thanks for that. The ClojureScript upgrade also required a JRE upgrade, so that may have been why I wasn't running the latest already. I am now on 2265 and Reagent is good to go.

My app is a mix between a little Reagent and a metric ton of pre-Reagent React usage, so the fact that it renders at all is a testament to Reagent's high quality.

@mike-thompson-day8
Copy link
Member

@holmsand I hate to hassle, but we seem to have stalled again. This release needs to be completed. Tell us what to do to help.

I've tried to come up with a list of the blocking issues:
#90 #88 #47

Also, I personally would like to see #81 addressed as part of the release.

Our experience: we've been using 0.5.0-alpha without issue. We have not found a use for wrap (but that is hardly surprising because we don't use cursors, etc).

.

@holmsand
Copy link
Contributor Author

@mike-thompson-day8 Thanks for the hassling (and the work on checking bug reports)!

I got a bit stuck on trying to decide whether the cursors should be in or out, and then I've been distracted by Real Work… You're of course absolutely right about the need for a release.

The big one of the bugs is, I suppose, #90. The new packaging alternative seems great, but I haven't had the time to try it out and understand the details yet. How easy is it, for example, to change or skip the React dependency if you supply the magic?

#47 is only an issue of documentation right now, isn't it? I added some tests to make sure that the "hack" should keep working (btw every time I see it, I like that hack a little bit more :)).

#81 is a bit more subtle. I think that just removing the assert may lead to memory leaks (since watches are added and never removed), so that needs to be thought through, and tested. But I definitely agree that it must be done.

#88 I haven't had the time to look at more closely. If someone could track down the problem, that would be great.

And then there are those pesky cursors... Right now I think that there should be cursors in Reagent core (since they seem to fit the way some people think, and pretty much the only way to make efficient cursors is to integrate them with core).

Oh, and there is a new version of React on the way. It might be interesting if it provides a faster alternative to createElement.

@robinheghan
Copy link

@holmsand What do you think of #87 ?

@seancorfield
Copy link
Member

My input:

I have no opinion on #81, #87, #88 as I haven't experienced any pain points yet from those.

Re: cursors - the status quo of one version in core and one version standalone is definitely not a tenable situation and we should make a decision sooner rather than later to avoid a community of developers growing up around use of the standalone cursor (and then deciding to rip it out).

I don't believe the two versions are even compatible right now: the standalone version has had a number of tweaks from @Frozenlock that make the behavior more consistent with an atom so at a minimum those should probably be added to the core version.

I can see an argument for maintaining a standalone version, separate from Reagent altogether, as being generally useful to ClojureScript (and, if I or someone else creates it, a version for Clojure) developers, but that would mean two versions to keep in sync which adds to maintenance. My only dog in this fight is that I want to use cursors somehow with Reagent, so if core is the best way to support a high-performance version, that's where I'd lean at this point. @Frozenlock do you want to create a PR on reagent with the same set of tweaks that have been applied to reagent-cursor?

@Frozenlock
Copy link

Okay, I've been thinking about this for quite a while now.

It will be a little long, but I think it's worth it to decide the project's direction.

Let's start with Reagent-cursor.
They way I see it, cursors/lenses/pointers are here to stay. They already surfaced in Om, Reagent, Javelin, Rum...

But it's a little more than that.

I think the IAtom protocol is a nice way to abstract any state.
It's really nice to have a widespread set of functions (deref, swap!, reset!, add-watch!...) that can be applied regardless of how we decide to store state.

Anything that respects the IAtom protocol is then compatible with everything that was made with atoms in mind. For example, storage-atom and Historian are both compatible with Reagent atoms and cursors without any modifications, even if they were created for vanilla atoms.

The more I think about it, the more it looks like @seancorfield 's original cursor (and the others like it) are just a specific case of a more general need. Our lord and savior @holmsand 's latest wrap shows a little more of this need: it adds a way to specify how we update the atom. I can easily see the same need for when we deref the atom.

The more general case of the current cursors would then be a way to specify a getter-fn and an update-fn. A way to transform how the data is passed from IAtom layers to the next.

As a trival example, say I always store numbers, but I'd like an IAtom interface that returns and accept strings instead.

(def my-atom (atom [1 2 3]))

@my-atom
;=> [1 2 3]

;; now lets assume a magic way to 'wrap' this atom
;; my-atom --> wrapper(get-fn: map-str, set-fn: read) --> string-atom

@string-atom
;;=> ["1" "2" "3"]

;; set new values...
(reset! string-atom ["3" "10"])

;; and the strings are automatically converted!
@my-atom
;;=> [3 10]

The VERY nice thing with layering IAtom is that each layer is totally opaque. I could be using a vanilla atom, or a SQL database on a remote server.

SQL ----> transit-encoding --> Internet ---> transit-decoding ----> IAtom(sate) ---> IAtom(string-atom) ---> React.js

Our code/application only has to be aware of this:

IAtom ---> React.js

It would be interesting to see if reagent-cursor can be modified to be able to generate more complex cursors.

(generate-cursor-type deref-fn swap-fn)

;; the current cursors are more or less this:
(def cursor (generate-cursor-type get-in update-in))

Reagent-cursor also has shown how the cursors are composable and can branch out in many sub-atoms. It makes testing components a whole lot easier because you don't need to have your entire app-sate, you can just mock-up a local cursor. From the point of view of the component, there's no difference.

Phew.

Okay, so how does this impact Reagent?

Reagent atoms are themselves only a wrapper for the vanilla atoms. They are effectively an IAtom layer, exactly like I talked about earlier. The problem is that they are always wrapping vanilla atoms.
Because of this, every time we want a slightly different behavior, we have to include it in core.

;; [harcorded] ;;

[atom --> reagent] ----> stuff with Reagent IAtom
[cursor --> reagent] ----> stuff with Reagent IAtom
[wrap --> reagent] ---> stuff with Reagent IAtom

It would be much better, IMO, to have a way to wrap any IAtom into a Reagent atom.

;; [harcorded] ;;

atom --> [reagent] ----> stuff with Reagent IAtom
cursor-type-1 --> [reagent] ----> stuff with Reagent IAtom
cursor-type-2 --> [reagent] ----> stuff with Reagent IAtom
SQL-backed-atom --> [reagent] ----> stuff with Reagent IAtom
Javelin-cell --> [reagent] ----> stuff with Reagent IAtom

Depending on how it needs to be implemented, the method to wrap the Reagent stuff could even be with the cursors!

(def reagent-atom (generate-cursor-type reagent-deref reagent-swap))

P.S.: Especially considering the all new React native, I think we should expect some atoms to be an abstraction on top of local databases/files.

Edit: (arg... ok, reagent atoms don't directly wrap vanilla atoms, but you know what I meant...)

@city41
Copy link

city41 commented Jan 28, 2015

I think fixing #88 is really important, getting CSS animation that easily is a great plus. I'm gonna spend the next hour or two looking into the bug, but since I'm new to the code base I can't make any hard promises.

@mike-thompson-day8
Copy link
Member

If there are technical reasons why Cursors needs to be in the core (better performance?), then let's just do that. Via OM, it is an established pattern.

Yes, using Cursors breaks down a bit at scale IMO (David Nolan himself says so in the advanced OM tutorial), but it is perfectly valid and useful for many kinds of apps. So let's agree to love Cursors and move on. :-)

@holmsand
Copy link
Contributor Author

holmsand commented Feb 2, 2015

Ok, cursor is here to stay. I've rewritten large parts of it to make it more flexible (and correct, for that matter). On top of that, I've switched the argument order (sorry @seancorfield :)) – I just could never remember that the atom should come second...

The flexibility part is that cursor now can take a function instead of an atom. That function will be passed the path argument for getting, and path and new-value for setting.

I.e. (given (def state (atom {}), these are roughly equivalent:

(r/cursor state [:foo])

and

(defn set-get
  ([k] (get-in @state k))
  ([k v] (swap! state assoc-in k v)))

(r/cursor set-get [:foo])

The setter/getter function can do arbitrary transformations, and even combine many atoms into a single view.

@seancorfield
Copy link
Member

That looks nice @holmsand and addresses a lot of the concerns / questions that @Frozenlock was asking about more generalized cursors!

@mike-thompson-day8
Copy link
Member

@holmsand Should we be testing the current state of master? Or are there more changes to land?

@holmsand
Copy link
Contributor Author

holmsand commented Feb 4, 2015

@mike-thompson-day8 No, some testing would be great! I had some more stuff planned, but I just realized that there's too much going on anyway :)

I'll try to update the examples, readme, etc., and push a beta (or rc?) in a couple of days.

@mike-thompson-day8
Copy link
Member

When you get a chance, would you mind publishing the latest to clojars (as alpha 2)? (In the meantime we'll lein install but the local maven repo is shared mutable state, and we know that is the root of all evil :-)).

@holmsand
Copy link
Contributor Author

holmsand commented Feb 4, 2015

Sure, there's now a reagent "0.5.0-alpha2" on clojars.

@holmsand
Copy link
Contributor Author

holmsand commented Feb 9, 2015

@mike-thompson-day8 Great list, thanks!

I think I've solved pretty much all of these, so I've pushed out a quick 0.5.0-alpha3 with these changes (mainly improved warning/error reporting, and a new reactify-component that creates an adapter for a Reagent component that can be used from JSX).

All the examples now use the new cljsjs packaging correctly – and all of them use figwheel!

Please have a look at 0.5.0-alpha3, and if there's nothing horribly wrong with it, we can get a beta out the door rather quickly.

@mike-thompson-day8
Copy link
Member

Thanks Dan!!! I've updated the issues list (above in a comment) to reflect the current state of play.

I'll be testing alpha3 today (although it now feels more like a beta).

If anyone else wants to test with their apps, now looks like the right time

@mike-thompson-day8
Copy link
Member

We have switched to using 0.5.0-alpha3 on 3 different apps. After one day of use, no problems found.

@Gonzih
Copy link

Gonzih commented Feb 13, 2015

Will cursor become smarter at some point? Would be nice to have component updates only if part of ratom that is in scope updates.

@Gonzih
Copy link

Gonzih commented Feb 13, 2015

Also I tried to update my project from alpha -> alpha3 and noticed that now reagent provides react which is great, but I still would like to use my own version of react with addons. What should I do in that case?

@city41
Copy link

city41 commented Feb 13, 2015

@Gonzih bring in reagent as [reagent "0.5.0-alpha3" :exclusions [cljsjs/react]]

I still think this is problematic though, because Reagent expects React to be provided via the classpath, ie the Reagent code does (require cljsjs.react). So as far as I can tell, you have to bundle React into your cljs bundle. I'd prefer to keep React separate and serve it from a CDN, I don't know if that is possible with this setup. Any ideas?

@Gonzih
Copy link

Gonzih commented Feb 14, 2015

I personally use webjars and preamble to include react in to my
resulting file. CDN might be good option. Anyway I think reagent should
not enforce any way to include react, but provide compatible version on
classpath as well.

On 02/13/2015 06:11 PM, Matt Greer wrote:

@Gonzih https://github.com/Gonzih bring in reagent as |[reagent
"0.5.0-alpha3" :exclusions [cljsjs/react]]|

I still think this is problematic though, because Reagent expects
React to be provided via the classpath, ie the Reagent code does
|(require cljsjs.react)|. So as far as I can tell, you have to bundle
React into your cljs bundle. I'd prefer to keep React separate and
serve it from a CDN, I don't know if that is possible with this setup.
Any ideas?


Reply to this email directly or view it on GitHub
#76 (comment).

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

But what about smarter cursors? What needs to be done there? Right now cursor is not very useful, more like syntactic sugar I guess.

@holmsand
Copy link
Contributor Author

@city41 @Gonzih If you have [reagent "0.5.0-alpha3" :exclusions [cljsjs/react]] in :dependencies, you'll have to provide the cljsjs.react namespace in some other way.

You can either add [cljsjs/react-with-addons "0.12.2-4"] as a dependency, or add a file named cljsjs/react.cljs (containing just (ns cljsjs.react)) to your project. In the latter case you'd obviously have to include React some other way (e.g from a CDN, or using :preamble) into your html page.

@holmsand
Copy link
Contributor Author

@Gonzih Cursors ought to be quite "smart" in alpha3. What exactly do you find un-smart about them? An example, preferably as a new issue, would be great.

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

Do components still update them-self if atom changes and changes are
outside of cursor's scope?

On 02/16/2015 04:15 PM, Dan Holmsand wrote:

@Gonzih https://github.com/Gonzih Cursors ought to be quite "smart"
in alpha3. What exactly do you find un-smart about them? An example,
preferably as a new issue, would be great.


Reply to this email directly or view it on GitHub
#76 (comment).

@holmsand
Copy link
Contributor Author

@Gonzih No, they should not. That wouldn't be very smart :)

As I said: If you experience unnecessary re-renderings (in 0.5.0-alpha3), then that is a bug and you should report that in a separate issue.

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

Ok thanks. I will benchmark my code again then!

On 02/16/2015 05:13 PM, Dan Holmsand wrote:

@Gonzih https://github.com/Gonzih No, they should not. That wouldn't
be very smart :)

As I said: If you experience unnecessary re-renderings (in
0.5.0-alpha3), then that is a bug and you should report that in a
separate issue.


Reply to this email directly or view it on GitHub
#76 (comment).

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

Oh! I just noticed that components are still updated if lens returns nil. Is that expected behavior?

@holmsand
Copy link
Contributor Author

@Gonzih Code example?

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

Example based on what is happening in my production code:

(defonce options (atom {}))
(defonce p1 (cursor options [:p1]))
(defonce p2 (cursor option [:p2]))

(defn some-component []
  ...
   @p1
   @p2
   ....
    (prn :i-got-evaluated))

So when I'm swapping options atom (not touching :p1 and :p2 keys) some-components is re-evaluated. But once @p1 and @p2 values are non-nil this behavior stops. I had no time today to test more cases. If it happens when both of them are not nil, if I need to set it once to non nil value or is it whenever they are set to nil. I can try to reproduce it outside of my code, but might take a long time. Do you need more details? I will investigate more tomorrow.

@holmsand
Copy link
Contributor Author

@Gonzih I can't reproduce that. Again: a proper bug report would be nice.

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

Ok, will do that

On 02/16/2015 08:14 PM, Dan Holmsand wrote:

@Gonzih https://github.com/Gonzih I can't reproduce that. Again: a
proper bug report would be nice.


Reply to this email directly or view it on GitHub
#76 (comment).

@mike-thompson-day8
Copy link
Member

At this point, I'd love to see a beta, or perhaps just a release and handle bugs via further point releases.

We've been using it as if it was a beta for a week now, without an issue.

@yogthos
Copy link
Member

yogthos commented Feb 16, 2015

👍 I've been 0.5 and haven't seen any issues either, I think a release would be nice at this point.

@Gonzih
Copy link

Gonzih commented Feb 16, 2015

@holmsand I tried to reproduce in new project - everything works like a charm. I'm assuming that it's an issue in my code for now then. Thanks for your replies!

@pandeiro
Copy link
Contributor

👍 for a 0.5.0 release, alpha3 working great in my project

@luposlip
Copy link

+1 for a 0.5.0 release from me. Have been using it in production for a while. No issues found.

Den 18/02/2015 kl. 18.25 skrev Murphy McMahon notifications@github.com:

for a 0.5.0 release, alpha3 working great in my project


Reply to this email directly or view it on GitHub.

@mike-thompson-day8
Copy link
Member

@holmsand if at all possible, it seems to me we need to get 0.0.5 out there before react-v0.13 gets released (currently at release candidate stage). As far as I can tell, the reagent v0.0.5 API is stable, and there are no known bugs. Seems as if we are all go. Let's not bother about beta. Seems to me we've been going through beta for a while now.

@holmsand
Copy link
Contributor Author

0.5.0 is now released (and is practically identical to 0.5.0-alpha3, except for documentation).

@mike-thompson-day8
Copy link
Member

Really good news. Thanks Dan!!!

@Frozenlock
Copy link

Thanks a lot!

@marcloyal3
Copy link

Awesome! @holmsand coincidentally, this comes just in time for our 1st Reagent SF meetup 👍

@yogthos
Copy link
Member

yogthos commented Mar 11, 2015

exciting news!

@yogthos yogthos closed this as completed May 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests