reagent-project / reagent

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

Reagent 0.6.0-alpha wants testing #212

Closed holmsand closed 8 years ago

holmsand commented 8 years ago

This is a quite big release, so be vigilant out there...

Check out the changelog, and http://reagent-project.github.io/news/news060-alpha.html for more info.

mike-thompson-day8 commented 8 years ago

I've not investigated deeply yet, but I'm seeing this from previously working code:

Error: Invariant Violation: findComponentRoot(..., .0.1.0.0.1.2.1.0.0.1:$360.1:$6): Unable to
find element. This probably means the DOM was unexpectedly mutated (e.g., by the browser), 
usually due to forgetting a <tbody> when using tables, nesting tags like <form>, <p>, or <a>, 
or using non-SVG elements in an <svg> parent. Try inspecting the child nodes of the element 
with React ID ``.

This turned out to be a fault in our application. React 14 does more checking and so it caught the error in our code.

If anyone wants to test their re-frame app, you'll have to use deps of [re-frame "0.7.0-alpha"] [re-frame "0.7.0"]

schmee commented 8 years ago

I'm trying to use react-select with Reagent 0.6.0. Here's my test component:

(defn test-button []
  [:> js/Select {:options [{:value "foo" :label "Foo"} {:value "bar" :label "Bar"}]
                 :name "Button"
                 :value {:value "foo" :label "Foo"}
                 :onChange #(println %)}])

~~The button itself works fine, but when navigating with the arrow keys, I get the error Uncaught TypeError: Cannot read property 'findDOMNode' of undefined from this line in react-select. The same thing happens regardless of what data I have in the button.~~

EDIT: found the problem, unrelated to Reagent.

tim-mccarthy commented 8 years ago

The changelog mentions an after-update function similar to next-tick, but I don't see it anywhere in the code.

yogthos commented 8 years ago

I found an interesting issue. I'm creating a bootstrap dropdown with the following code:

(def modal? (atom false))

(defn modal []
  (fn []
    [:div
     [:div.modal-dialog
      [:div.modal-content
       [:div.modal-header [:h3 "Modal Header"]]
       [:div.modal-body "I'm a modal"]
       [:div.modal-footer
        [:div.bootstrap-dialog-footer
         [:button.btn.btn-danger
          {:on-click #(reset! modal? false)} "Ok"]]]]]]))

(defn home-page []
  [:div
   [:div.container>div.row>div.col-md-12
    [:h2 "modal-test"]
    [:li.dropdown.open       
       [:a.dropdown-toggle        
        [:ul.dropdown-menu
         [:li>a {:on-click #(reset! modal? true)}
          "action 1"]
         [:li>a {:on-click #(println "action 2")}
          "action 2"]]]]    
    (when @modal?
      [modal])]])

This produces a warning:

Warning: validateDOMNesting(...): <a> cannot appear as a descendant of <a>. See reactid_duplication.core.home_page > a > ... > a.

I'm assuming this comes from React 0.14, and doesn't seem to affect anything functionality wise. However, this also produces a duplicate reactid:

Uncaught Error: Invariant Violation: ReactMount: Two valid but unequal nodes with the same `data-reactid`: .0.0.0.0.1.0

Then when first action "action 1" link is clicked a modal pops up, the modal pops up, but clicking the button on the modal produces the above error again and the modal is never removed. Clicking the "action 2" link in this state will still print to console.

Everything works as expected with Reagent 0.5.1. I created a sample project here with the issue.

mike-thompson-day8 commented 8 years ago

@holmsand Issues #201 #158 are pretty critical to re-frame. What are your thoughts about them being included in v0.6.0? I'll happily do a PR for them, but I don't want to spend time if the patches are unlikely to be successful in the near term (historically, Reagent PRs have had a very low chance of success).

Regarding this release: with its various breakages, I feel like it is going to be all pain and little gain for my apps. So I'm not looking forward to this. And, as someone involved actively in supporting reagent and re-frame on a daily basis, I feel like my task just got harder, with reagent now appearing to sprout something called dispatch (via the new docs) which is semantically different to re-frame's dispatch (but similar), etc. Muddier waters. More hassle and confusion. Like I said, all pain and no gain from my point of view, and it seems to ignore long outstanding, more pressing issues like, for example, #178 #144 #154 #209.

So while I'm very grateful for reagent, I'm disappointed with the level of consultation around what goes into a release and worried about future direction. Things like wrap in the last release seemed to come from left of field, and not be grounded in genuine usecases (to my knowledge). And even track this time is unexpected and interesting, but it is also too high level for me to use -- I need a lower level feature. I would love to have had a chance to talk the design through.

So, yeah. I know this is OS and all that, so we have lots of choices, but I thought I'd try to give as much straight-talking feedback as I could, so you know where other people's heads are at.

smee commented 8 years ago

The assert in https://github.com/reagent-project/reagent/blob/master/src/reagent/dom/server.cljs#L12 is only annoying at development time, but as soon as I use advanced compilation modes, the code stops running here, because ReactDOMServer is not available. Since this code supposedly is not intented to run in a browser setting I would suggest to remove it?

holmsand commented 8 years ago

@yogthos Interesting problem (as in: took some time to figure out...).

Turns out this is not a Reagent problem, nor strictly speaking a React one. React (quite reasonably) expects that the nodes it inserts will remain where it put them, and (especially) not moved or duplicated.

Your code, however, results in duplicate <a> nodes (due to the "unorthodox" nesting, and your putting the block-level <ul> inside an <a> that expects inline elements only). Here's a somewhat minimal HTML example that shows what happens:

<!doctype html>
<div>
  <a id="duplicated">
    <ul>
      <li><a>Bar</a></li>
    </ul>
  </a>
</div>

The browser will silently convert this into the slightly more valid

<div>
  <a id="duplicated">
  </a><ul><a id="duplicated">
    </a><li><a id="duplicated"></a><a>Bar</a></li>
  </ul>
</div>

The duplication of the a:s is what confuses React, which (quite rightly) throws an exception (and gives a hint about the root cause).

To avoid this issue you would have to avoid nesting the a:s, for example like this:

(defn home-page []
  [:div
   [:div.container>div.row>div.col-md-12
    [:h2 "modal-test"]
    [:li.dropdown.open
     [:a.dropdown-toggle]
     [:div.dropdown.open
      [:ul.dropdown-menu
       [:li>a {:on-click #(reset! modal? true)}
        "action 1"]
       [:li>a {:on-click #(println "action 2")}
        "action 2"]]]]
    (when @modal?
      [modal])]])
yogthos commented 8 years ago

Interesting, this examples comes from implementing bootstrap components. Their CSS styling is very specific, so it ends up requiring nesting elements that shouldn't be nested normally. I wonder why this worked in 0.13 at all then.

yogthos commented 8 years ago

So, good news is that Bootstrap 4 changed the way dropdowns work. It no longer necessitates invalid element nesting, and looks like it's compatible with the latest React. This is the only issue I've run into testing my apps, and it doesn't appear to be Reagent specific.

Frozenlock commented 8 years ago

I've tested 6.0.0-alpha on a few of my apps and everything is working fine.

dmarjenburgh commented 8 years ago

We've used reagent 0.6.0-alpha in production for 1.5 months. Lots of warnings concerning valid domnesting started popping up. These were all from the upgraded React though (which was helpful in cleaning it up). We are using the track and with-let as well without any problems so far.

Deraen commented 8 years ago

At Metosin we have several apps in production using 0.6.0-alpha with no problems. No problems with new projects either. New projects are using track heavily. In addition to pure Reagent projects we have at least one re-frame project using 0.6.0-alpha.

mike-thompson-day8 commented 8 years ago

We've done a first round of testing on a few re-frame apps, using [re-frame "0.7.0"], and they all appear to work. So that's encouraging. More careful testing to follow shortly.

mike-thompson-day8 commented 8 years ago

I can confirm that we are now running two (large-ish) re-frame apps in production using 0.6.0-alpha

esp1 commented 8 years ago

Any update on when 0.6.0 will be released, and if it will come out with React 0.14 or 15?

mike-thompson-day8 commented 8 years ago

On clojurians Slack, I have just asked for everyone to try [reagent "0.6.0-SNAPSHOT"] which uses react 15. This SNAPSHOT is otherwise the same as [reagent "0.6.0-alpha"] which uses React 0.14.

I have asked testers to report their experience in this ticket. So the current plan is: if everything goes smoothly, the 0.6.0 release will include React 15, not 0.14. Perhaps in about a week (subject to some code which Dan wants to land).

* Please test * on working apps now. Report your experience here.

smee commented 8 years ago

0.6.0-SNAPSHOT works like a charm. I updated https://github.com/cljsjs/packages/tree/master/react-bootstrap and tested both in combination, with :none and :advanced compilation modes

edvorg commented 8 years ago

Updated this one to 0.6.0-SNAPSHOT. Works just fine with no warnings.

mccraigmccraig commented 8 years ago

i'm getting a problem with Uncaught Error: js/ReactDOM is missing

i have a custom build of react https://github.com/employeerepublic/react-with-addons-and-tap-event-plugin to include the https://github.com/zilverline/react-tap-event-plugin ... this was fine with 0.6.0-alpha and 0.14.7, but no longer

mccraigmccraig commented 8 years ago

lol, ok, i found the problem - react-dom doesn't create the ReactDOM object, it just copies a ref from React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED , so all i have to do is create the React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED in my custom build

https://github.com/employeerepublic/react-with-addons-and-tap-event-plugin/blob/master/react-with-addons-and-tap-event-plugin.js

i hope i'm not gonna get fired now

mccraigmccraig commented 8 years ago

after that, 0.6.0-SNAPSHOT seems to work fine

hugooliveirad commented 8 years ago

updated private 9kloc re-frame project: everything is green and looking fine!

fasiha commented 8 years ago

Upgraded internal 2kloc (half Clojure, half ClojureScript) re-frame project to [reagent "0.6.0-SNAPSHOT"] [re-frame "0.7.0"]. All seems well—no warnings, no changes in behavior. Thanks for everyone's hard work, I cannot imagine having done this in plain JS.

arichiardi commented 8 years ago

I bumped clojurescript.io and no problem at the moment, I have a warning but I don't think it is from reagent (I also use re-frame and re-com, I will investigate:

WARNING: Use of undeclared Var cljs-time.format/constructor at line 413 /home/kapitan/.boot/cache/tmp/home/kapitan/git/cljs-repl-web/57h/fidlf9/main.out/cljs_time/format.cljs
mccraigmccraig commented 8 years ago

just seen a warning

Warning: ReagentInput is changing a controlled input of type radio to be uncontrolled. 
Input elements should not switch from controlled to uncontrolled (or vice versa). 
Decide between using a controlled or uncontrolled input element for the lifetime of the 
component. More info: https://fb.me/react-controlled-components
  warning @ react-with-addons.inc.js:1221
  ReactDOMInput.updateWrapper @ react-with-addons.inc.js:9731
  forceUpdateIfMounted @ react-with-addons.inc.js:9631
  _assign.notifyAll @ react-with-addons.inc.js:2553
  flushBatchedUpdates @ react-with-addons.inc.js:15404
  wrapper @ react-with-addons.inc.js:14174
  Mixin.closeAll @ react-with-addons.inc.js:17671
  Mixin.perform @ react-with-addons.inc.js:17618
  ReactDefaultBatchingStrategy.batchedUpdates @ react-with-addons.inc.js:11008
  batchedUpdates @ react-with-addons.inc.js:15328
  ReactEventListener.dispatchEvent @ react-with-addons.inc.js:12507

my code in this area hasn't changed since upgrading to 0.6.0-SNAPSHOT so my guess is it's something to do with reagent & react-15

here's the hiccup causing the warning - nothing particularly special, and it still seems to work fine despite the warning

[:div.form-group.type.radio-list

 [:div.radio
  [:label
   [:input
    {:type "radio" :name "type" :value "group"
     :id (str type-field-id "-1")
     :checked (= "group" type)
     :on-change (fn [e]
                  (let [v (-> e .-target .-value)]
                    (dispatch [:assoc-edit-conversation
                               :type
                               v])
                    (dispatch [:conversations/edit-conversation-flow])))}]
   [:i.icon-chat-bubble.icon-md-block]
   "Just a Yap"]]

 (when (or choose-addressees
           focus-group-location?)
   [:div.radio
    [:label
     [:input
      {:type "radio" :name "type" :value "shiftswap"
       :id (str type-field-id "-3")
       :checked (= "shiftswap" type)
       :on-change (fn [e]
                    (let [v (-> e .-target .-value)]
                      (dispatch [:assoc-edit-conversation
                                 :type
                                 v])
                      (dispatch [:conversations/edit-conversation-flow])))}]
     [:i.icon-symbol-arrows.icon-md-block]
     "Shift swap"]])]
Deraen commented 8 years ago

@mccraigmccraig Could be related to using both value and checked with radio input. I would just set the value in on-change handler manually so I wouldn't need to use name or value properties.

mccraigmccraig commented 8 years ago

@Deraen yep, that fixes it. thanks for the fresh eyes :)

Deraen commented 8 years ago

Could also be a bug in React. I tried checking the logic here but I don't see why using both should trigger the warning: https://github.com/facebook/react/blob/36e4fe54a801a325d30359653e588e9705e7532b/src/renderers/dom/client/wrappers/ReactDOMInput.js#L189-L204

mike-thompson-day8 commented 8 years ago

We have released a new version of re-com (0.8.3) which works with the new Reagent SNAPSHOT. React 15 is stricter than React 0.13, producing more warnings, so we had to clear them up. But no problems other than warnings.

mike-thompson-day8 commented 8 years ago

We've got through to using the recent -SNAPSHOT in production without incident. Our guy who did the transition swears that the apps are now snappier.

mike-thompson-day8 commented 8 years ago

The transition to React 15 appears to have been a success, There's now a [reagent "0.6.0-alpha2"] available. We are waiting on one more code drop from Dan. We will then cut a "release candidate".

benhowell commented 8 years ago

Hi all, I'm having a problem with the following error when trying to mount my app using 0.6.0-alpha2: Error: js/ReactDOM is missing

Previously using 0.5.1 with no problems

Deraen commented 8 years ago

@benhowell Check your dependencies tree (lein deps :tree or boot show -d). Something might be depending on the old React.

benhowell commented 8 years ago

@Deraen Thanks heaps, that's pinned down the problem. My question now is, if I use :exclusions [cljsjs/react] on reagent et al. will this will mean I'm stuck using react 0.13.3-1? If that's the case then I'll replace my react-quill components instead.

[cljsjs/react-quill "0.3.0-0"] -> [cljsjs/react "0.13.3-1"] overrides [re-com "0.8.3"] -> [reagent "0.6.0-alpha"] -> [cljsjs/react-dom-server "0.14.3-0"] -> [cljsjs/react "0.14.3-0"] and [re-com "0.8.3"] -> [reagent "0.6.0-alpha"] -> [cljsjs/react-dom "0.14.3-1"] -> [cljsjs/react "0.14.3-0"] and [reagent "0.6.0-alpha2"] -> [cljsjs/react-dom-server "15.0.2-0"] -> [cljsjs/react "15.0.2-0"] and [reagent "0.6.0-alpha2"] -> [cljsjs/react-dom "15.0.2-0"] -> [cljsjs/react "15.0.2-0"]

Consider using these exclusions: [re-com "0.8.3" :exclusions [cljsjs/react]] [re-com "0.8.3" :exclusions [cljsjs/react]] [reagent "0.6.0-alpha2" :exclusions [cljsjs/react]] [reagent "0.6.0-alpha2" :exclusions [cljsjs/react]]

Deraen commented 8 years ago

@benhowell You should probably exclude cljsjs/react from react-quill. Or update react-quill package. I would only use exclusions when if react-with-addons is required.

benhowell commented 8 years ago

Thanks for all the help @Deraen . Unfortunately react-quill bombs out on some functions that have been changed for react 14. I've been considering throwing react-quill away in favour of native quill anyway.

Sorry for the disruption as this turns out to be nothing to do with reagent, but I really do appreciate reagent! Thanks :)

mike-thompson-day8 commented 8 years ago

With v0.6.0 now released, I'm closing.