reagent-project / reagent

A minimalistic ClojureScript interface to React.js
http://reagent-project.github.io/
MIT License
4.75k stars 414 forks source link

Renaming atom to ratom #161

Closed danielcompton closed 9 years ago

danielcompton commented 9 years ago

EDIT: This issue was opened to address the common issue of people using Atoms instead of Ratoms. Initially it was to investigate the possibility of logging a warning when users rendered a Clojure atom instead of a Ratom. That was deemed infeasible, but the much easier renaming of atom to ratom came up as another avenue. I'm editing this issue rather than creating a new one as there was a lot of good discussion around renaming atom to ratom.


ORIGINAL: Is it worth logging a warning to the user when they use an atom instead of a ratom in Reagent rendering code? There's arguments to both sides, so I'm opening up an issue for discussion.

Frozenlock commented 9 years ago

I'm still using vanilla atoms with Reagent.

Do you have a way of distinguishing an accidental atom versus a legitimate one? What would be the overhead?

danielcompton commented 9 years ago

I'd check if it implemented IReactiveAtom at first blush. It could probably be eliminated at prod time through Google Closure dead code elimination. Cost at dev time would be pretty small too.

Frozenlock commented 9 years ago

But how would that find accidental atoms? Like I said, I still use vanilla atoms when I don't need it to update a component.

danielcompton commented 9 years ago

It depends what you mean by accidental. Would need some thought for your use case. What's the benefit of using Clojure atoms over just using ratoms everywhere?

Frozenlock commented 9 years ago

Less overhead. Better control on what will trigger an update.

There might also be cases where one has no choice. Other atom implementations can exist, and Reagent does not allow them to be reagent-ized. (See #92)

Assuming everyone will always use ratoms is... well... a huge assumption. :smile:

GetContented commented 9 years ago

@Frozenlock I agree. @danielcompton I don't like this idea much. It feels too magic, and takes control away from the programmer.

danielcompton commented 9 years ago

A few things:

  1. After talking with colleagues, I'm not sure how this could actually be implemented.
  2. We're only talking about logging a warning, I'm not sure what's magical about that?
  3. There would probably need to be a flag you could set that says "I know what I'm doing, don't warn me".
  4. Another route to take would be to look at renaming atom to ratom, so people could never use one instead of the other accidentally. This would need to go through a deprecation process of course.
mike-thompson-day8 commented 9 years ago

There's a very steady stream of people who get caught by this, and it is a baffling problem to solve.

But equally there is absolutely no way to solve this technically. There is no way to know if an clojure.core/atom is being referenced where a reagent.core/atom is intended.

The real solution here is, indeed, to rename reagent.core/atom so the confusion can never happen in the first place, and, yes, reagent.core/ratom strikes me as the right name.

GetContented commented 9 years ago

Clojure has namespaces, why not just use them? Use namespace aliasing in preference of refering entire namespaces, and you'll be fine. Import the namespace reagent.core as r, and then you can write r/atom... simple, same thing with r/cursor. It's unambiguous, clean and obvious what you mean.

mike-thompson-day8 commented 9 years ago

Yeah, I really do understand about namespaces :-) but people do get regularly caught by this as is evidenced by comments in clojarians and other places.

IMO it is a bug in the API that a clojure.core symbol was sublty repurposed. It makes people prone to a silent and baffling problem.

Deraen commented 9 years ago

It would be possible to check if atom is normal or ratom by checking if it satisfies IReactiveAtom protocol.

GetContented commented 9 years ago

Of course you do. :)

I disagree with you, tho about the API: IMO it's not a bug. This particular atom is reagent.core/atom and is very similar, but different to clojure.core/atom. If every library had to consider other namespaces, we'd end up with a lot of very long names. That would be unfortunate. Also, to a new person, who's to say the name ratom isn't something that evokes rat-om rather than r-atom? It's a reactive atom. That's what it is.

Rather than change API, work on making the documentation and tutorials clearer. It's not a great idea to blindly refer entire namespaces, or even refer them at all (IMO). This problem just doesn't manifest if you alias all your namespaces. It's a very good habit to get into, and makes you really think about where the code you're using is coming from.

I do realise I'm preaching to the converted a little, but this is very important generally, not just for reagent, but clojure in general. Nice namespace aliasing is something that very few languages have done right, and we have an excellent implementation of them. We should embrace them rather than "try to make things easier". Clojure is about understanding what we're doing, and choosing radically simpler ways of doing things with full appreciation for the ramifications of those choices, not making things easier for a variety of folk. When things become easier for a set of people, they naturally become harder for another set. By embracing simplicity first, we adjust for an objective metric rather than something focussed on subjectivity.

However, I'm all for better documentation and tutorials. I personally found the function-returning-function semantics somewhat distrurbingly tricky at first, but I persisted with it. I wouldn't like to see it adjusted simply because it was a bit tricky for beginners - it's immensely useful, and a good simple solution. However, better docs around it would be excellent and alleviate those problems for beginners.

@Deraen It's totally possible to do that, but how do you detect every single atom in and app and warn that they don't satisfy that protocol, without a performance hit in either dev or production?

mike-thompson-day8 commented 9 years ago

We come from different perspectives on this. I take a practical point of view. If an API causes programmers to regularly suffer a baffling problem, then there's a bug with the API.

I take the same view on the apps I design. If I get support calls from puzzled users, I regard it as a bug with my app design. Consistent usage problems are as good as bugs!

But I get that many might not agree.

Deraen commented 9 years ago

@JulianLeviston Ah, I didn't think this through. I was thinking checking the atoms before deref but as the deref happens in user's code so that's not possible.

GetContented commented 9 years ago

@mike-thompson-day8 I agree with you about customers, wholeheartedly, but programmers are not customers. You can stretch the analogy to them if you like, but I think it's a little fraught.

Perhaps, if programmers are having a problem with an API it just says that programmers are having problems with the API, it doens't necessarily mean the API is bad. There's rarely a single correct answer. If people have trouble with category theory or fourrier analysis, perhaps we need to educate people in better ways rather than change what category theory or fourrier analysis are.

People coming from Ruby or Smalltalk to Clojure often have a big problem with the name of the function "some" (which is kind of like 'detect' in SmallTalk and Ruby), but that's not necessarily a good reason to change it. Perhaps those people need to realise that Clojure is quite different to SmallTalk or Ruby and really understand how and why.

We don't want to end up with a convoluted system that pulls this way and that way depending on the seasons and fashions of time.

I guess I'll put my agenda and attempts to convince you away for now though, because it's probably not the place. I do hope that we don't end up with "ratom", though.

Frozenlock commented 9 years ago

Just like @JulianLeviston, my code is filled with r/atom.

If every library had to consider other namespaces, we'd end up with a lot of very long names.

Coming from an Emacs background, this is a very bad situation indeed.

Anyway, having ratom is not really what this issue was opened for. I would invite people wanting it to open another issue.

mike-thompson-day8 commented 9 years ago

@Frozenlock This issue isn't about your code being filled with r/atom or not.

And I'm not sure why you feel this discussion has to be had in another issue. @danielcompton raised this issue for discussion because recently, on clojarians, yet another person said how they'd spent hours being baffled by this issue. I promise you this issue comes up all the time. Once a week?

So what better place than here to discuss possible solutions? Detecting non-ratom use is one suggestion - which, I believe, isn't technically possible, and probably isn't desirable. So I'm floating another solution.

@JulianLeviston I always regard consumers of an API as customers. So I absolutely don't agree with you that programmers are different from customers. They are people using some technology. If that technology baffles them for a time, and causes them pain, then that's a bug.

And if I believed the root cause of the pain was the shadowing of clojure.core/atom then I'd attack that root cause.

Certainly something we can all agree on is that the docs should highlight this as a current problem. Mind you, how often are docs properly read by someone charging in?

Frozenlock commented 9 years ago

@mike-thompson-day8

This issue isn't about your code being filled with r/atom or not.

I never said it was. I was providing additional evidence as one natural solution to what some consider a problem.

And I'm not sure why you feel this discussion has to be had in another issue.

Because the issue was about whether or not a warning should be printed to the user when he was using anything else than a Reagent atom. This is VERY different from adding a ratom function. Everyone reading the issues list couldn't have known about the shift in discussion. To use your own argument, it's about the users. :smile:

Tho now OP changed the issue's title, as well as the opening paragraph, so I guess the point is no longer relevant.

danielcompton commented 9 years ago

The API a library presents is UX design. Just like all other design disciplines, API design should be empathetic towards users and avoid giving users the opportunity to make mistakes.

Clojure programmers are no smarter than any other programmers, and judging by the steady stream of people that make a mistake using atom instead of ratom, I think there is a good argument for renaming atom to ratom. Ratom is a more descriptive term than atom, and if you look at this discussion and further afield online you will see people use ratom to avoid collision with atom. Heck, even the namespace is reagent.ratom and the deftype is RAtom.

This bug is particularly malicious because a ratom works so similarly to an atom that things can work fine using an atom until your app starts behaving in unexpected ways down the track. The large distance between when the mistake was made and when it could be discovered makes this a lot harder to track down.

If every library had to consider other namespaces, we'd end up with a lot of very long names.

Every library doesn't need to consider other namespaces, but it does need to consider clojure.core and the vars that are in that namespace by default for every single Clojure namespace. Also, I'm not recommending we call it reactive-reagent-atom, I'm recommending adding a single character.

We should embrace them rather than "try to make things easier". Clojure is about understanding what we're doing, and choosing radically simpler ways of doing things with full appreciation for the ramifications of those choices, not making things easier for a variety of folk. When things become easier for a set of people, they naturally become harder for another set. By embracing simplicity first, we adjust for an objective metric rather than something focussed on subjectivity.

Clojure embraces simple over easy, but it doesn't preclude making things easier either :). I don't see things getting harder for a lot of folk by changing atom to ratom (after the original deprecation).

I personally found the function-returning-function semantics somewhat distrurbingly tricky at first, but I persisted with it. I wouldn't like to see it adjusted simply because it was a bit tricky for beginners - it's immensely useful, and a good simple solution.

This isn't a good analogy, we're talking about changing API naming to make it less error prone. Some things have inherent complexity to them, and you just need to get over the hump to understand them. Using atom instead of ratom is incidental complexity that can be addressed.

but programmers are not customers. You can stretch the analogy to them if you like, but I think it's a little fraught.

Clojure programmers are no smarter than anyone else, we make mistakes just like everyone else. API design needs to consider users just like every other branch of design. In any other discipline if users were commonly making a mistake, the designers would look to prevent it. Documentation is one way to address this, but the best documentation page is the one that never needs to be written.

Perhaps, if programmers are having a problem with an API it just says that programmers are having problems with the API, it doens't necessarily mean the API is bad. There's rarely a single correct answer. If people have trouble with category theory or fourrier analysis, perhaps we need to educate people in better ways rather than change what category theory or fourrier analysis are.

Category theory and Fourier analysis aren't API's, they're branches of mathematics. If programmers are having a problem with an API, it doesn't always mean that the API is bad. However it does deserve to be investigated. Examining the root cause of this category of mistakes shows people are using atom when they meant ratom.

Perhaps those people need to realise that Clojure is quite different to SmallTalk or Ruby and really understand how and why.

Nobody is saying that Clojure needs to be SmallTalk or Ruby. Clojure's differences from SmallTalk and Ruby have almost nothing to do with the names that it uses.

We don't want to end up with a convoluted system that pulls this way and that way depending on the seasons and fashions of time.

Atom has been in Clojure since 1.0. We're not pulling this way and that depending on seasons and fashions, we're adjusting to real world usage and addressing a very real API issue.

P.S. I've renamed this issue as there are a lot of good comments here about renaming atom to ratom.

Frozenlock commented 9 years ago

I have much less strong opinions about renaming reagent atoms than printing warning messages.

Now, as the renaming option is on the table, should it be renamed to something even more different? Wouldn't people still quickly read ratom as atom?

GetContented commented 9 years ago

@danielcompton @mike-thompson-day8 We agree on all points. I just wonder WHY consumers of this particular API are getting confused. That concern should be addressed, at the root. I can't seem to understand how people who know how to use clojure can get caught up by this.

I've been caught by this before, but it's actually more subtle than you would think. When I chose an atom in that case (clojure.core/atom), I actually did choose the thing I wanted. No amount of naming would have fixed that for me. I picked the wrong structure because I forgot I wanted a reagent reactive atom in that case.

Having said this, if you guys thing that renaming it will solve this at the root (which by the way could cause quite a few people quite a lot of annoyance as they'll have to refactor all of their extant code... unless it's aliased to reagent.core/atom)... well, perhaps you're right.

It's also very interesting, because, really, the confusion stems from the fact that there should only be one atom. Reactive atoms are really not something different. They're only different because of the requirements of the context. If one puts an atom in a Reagent view, then one is wanting it to be reactive, just like if you put a function symbol into a reagent view. Now this might have been what @danielcompton was talking about all along, in which case I agree with him... if you try to render with a dereffed clojure.core/atom in the render vectors of your reagent code, I'd say 99% of the time you actually meant to use a reagent atom or cursor.

It's a bit subtle, because the job reagent atom does is to wrap an atom in a reaction. I can't help but wonder why rather than specify the reaction at the atom level, why can't we impose the reaction on the atom on an ex-post-facto basis... that is, have a special reagent dereffing operator/syntax/function (or if it's possible overload deref in this context)... that registers the atom with a reaction wrapper on first call/render of the atom, and then subsequenty there will be no problem with picking "which atom - reagent or clojure core?" that was intended - because there will only be one atom. Mind you I don't know the internals of the probject well enough to really recommend this idea as valid or possible. @mike-thompson-day8 I'm reckoning you would?

The rule would become... deref an atom/cursor/wrap in a reagent render and you'll naturally get a reactive atom, or cursor, or wrap.

Alternatively, we could simply abandon atom altogether and just have a "full cursor" - that is, one with a root path of nothing (which I don't think works right now, but I haven't actually ever tried it, I don't think). This idea fits with me really well - I'm actually pretty sure I've needed this in the past - to have an atom, but also to have a reagent wrapped version of it that works separately). You could do this today by using wrap... just stop referring to reagent.core/atom and use cursors everywhere for everything - this would just be a matter of documentation change.

mike-thompson-day8 commented 9 years ago

@JulianLeviston For example, the reagent README says to do this:

(ns example
  (:require [reagent.core :as reagent :refer [atom]]))

followed by this:

(def my-html (atom ""))

If you forget the first bit (easily done) BINGO - baffling problem. Obviously the docs could be improved, but for many people this is all too easy to do. (And please no more lectures about r/atom , we do get it honestly, I just care about not answering this question any more).

GetContented commented 9 years ago

@mike-thompson-day8 We're in agreement. I fully understand your points.

Sorry you thought I was lecturing you about namespaces. It wasn't for you, it was for daniel. Yes, I agree that it's bad documentation. It's the primary way people find out about Reagent, and it should be changed. I think you're excellent at documentation writing, by the way ;-)

Referring sucks because it creates confusing code. It's very common practice in Haskell, too, which leads to a lot of very difficult to understand code for new people. (note I'm not even necessarily talking about newbies here - if I'm new to a particular codebase that hasn't used something I've seen before, and it's using several imported namespaces which are the clojure equiv of full referred namespaces, it makes it non-obvious which parts come from which namespaces. That's not clear, no matter HOW good you are the language's syntax).

mike-thompson-day8 commented 9 years ago

@holmsand and @GetContented have updated the docs so they now encourage use of r/atom. That will help many users to avoid this issue in the future.

Which leaves open one question: should we change the name of atom to ratom in order to absolutely remove any chance of problems?

I'm guessing from the discussion above and Dan's actions to date that the vote would be "no". Backwards compatibility and all that. So I'm going to close this. But I stand ready to reopen this at a moments notice if there are sufficient "yes" voices (for the record, my vote would be "yes").