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

React-16: Supported Version #75

Open
bteepell opened this issue Jan 15, 2018 · 10 comments
Open

React-16: Supported Version #75

bteepell opened this issue Jan 15, 2018 · 10 comments

Comments

@bteepell
Copy link

Do you have any plans to upgrade supported React version?

@njordhov
Copy link

It would be a step forward if kioo.util/WrapComponent didn't call the no longer supported React.createClass at loading time but rather delayed the call until first use by using e.g. memoize. That way, we may use create-react-class for a smoother transition to React-16 even before kioo is fully upgraded.

@ckirkendall
Copy link
Owner

I do plan on supporting react 16. I need to spend some time thinking about the memoize idea but I like the idea.

@njordhov
Copy link

njordhov commented Apr 26, 2018

The proposal enables releasing an interim version of kioo that uses React 15 yet allows developers to override with React 16. The changes are minor and should have ignorable impact on performance, only postponing the React calls from loading time to once at runtime. In brief, the current WrapComponent may be changed to:

(defn WrapComponent [props]                                                                                     
  ((wrap-component-fn) props))

where wrap-component-fn is a memoized fn contaning the current WrapComponent code:

 (def wrap-component-fn
   (memoize 
     (fn []
       (.createFactory js/React  
         (.createClass js/React 
         ....                                                                                 

@njordhov
Copy link

Also update dependencies to [sablono "0.8.1"] or later as 0.8.0 still calls React.createClass at loading time.

@jocrau
Copy link

jocrau commented Apr 26, 2018

I am not sure if memoizing the factory is necessary here. WarpComponent is called in handle-wrapper with a props argument that is a JavaScript object. Memoization stores the value the function evaluates to in a map using the (sequence of) arguments it was called with as key. But the subsequent props are never identical or even equal to any props stored in the memoization map, because (= (clj->js {:foo 1}) (clj->js {:foo 1})) => false.

@jocrau
Copy link

jocrau commented Apr 26, 2018

There is currently a mix of incompatible libraries and it is difficult to find the right way to upgrade. The dimensions of complexity are:

  • cljsjs vs npm
  • React v15 vs v16
  • Use of create-react-class vs. inherit from React.Component

Sablono 0.8.4 currently goes the route of [cljsjs, v16, inherit]. For kioo in the short term the combination of [cljsjs, v16, create-react-class] might be the best. In the long run [npm, v16, inheritance] seems to be the future - at least to me. What are your thoughts?

@njordhov
Copy link

njordhov commented Apr 26, 2018

@jocrau Note that the proposed memoization is on a function with zero arguments returning a function that takes props as argument. Hence the memoized function is called just once, with the function returned being called with the different props. No props are stored in the memoization map.

@jocrau
Copy link

jocrau commented Apr 27, 2018

@TerjeNorderhaug You are right. Memoization will cause React.createFactory to only be called once.

I will try to update Kioo to use create-react-class and send a pull request.

@ckirkendall
Copy link
Owner

@TerjeNorderhaug That would be awesome!

@rastandy
Copy link

Same problem here.
Would love to see this fixed and continue to use this great library.

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 a pull request may close this issue.

5 participants