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

Add IWithMeta protocol to RAtom #314

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Conversation

frerom
Copy link
Contributor

@frerom frerom commented Oct 6, 2017

Fixes #257

@danielcompton
Copy link
Contributor

danielcompton commented Oct 6, 2017

This would return a new instance of the Ratom when calling with-meta. This seems idiomatic amongst other core Clojure code, but I'm not sure what its effects on Reagent would be, both because Reagent makes heavy use of metadata, and because object identity is quite important in Reagent, and could lead to confusing scenarios for rerunning dependent changes. It might be fine, but this should probably be checked out further.

@Deraen
Copy link
Member

Deraen commented Oct 20, 2017

I think this will be safe. Reagent doesn't change RAtom meta ever, so only case where new RAtom is created using with-meta should be when user does so. And I think it won't be easy to do that accidentally, unless one has two nested RAtoms or such.

(deftest ratom-with-meta
  (let [value {:val 1}
        meta-value {:meta-val 1}
        state (r/atom (r/atom value))]
    (is (= (meta @state) nil))
    (swap! state #(with-meta % meta-value))
    (is (= (meta @state) nil))   
    )) 

@Deraen Deraen merged commit 0b38b44 into reagent-project:master Oct 20, 2017
@frerom frerom deleted the with-meta branch October 20, 2017 22:15
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

Successfully merging this pull request may close these issues.

3 participants