Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Remove createEagerFactory and createEagerElement #538

Merged
merged 1 commit into from
Oct 6, 2017
Merged

Remove createEagerFactory and createEagerElement #538

merged 1 commit into from
Oct 6, 2017

Conversation

deepsweet
Copy link
Contributor

closes #489, as discussed.

@codecov-io
Copy link

Codecov Report

Merging #538 into master will decrease coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
- Coverage   88.94%   88.37%   -0.57%     
==========================================
  Files          53       49       -4     
  Lines         389      370      -19     
==========================================
- Hits          346      327      -19     
  Misses         43       43
Impacted Files Coverage Δ
src/packages/recompose/defaultProps.js 85.71% <100%> (ø) ⬆️
src/packages/recompose/mapPropsStream.js 83.33% <100%> (ø) ⬆️
src/packages/recompose/withReducer.js 90.9% <100%> (ø) ⬆️
src/packages/recompose/shouldUpdate.js 85.71% <100%> (ø) ⬆️
src/packages/recompose/renderComponent.js 100% <100%> (ø) ⬆️
src/packages/recompose/lifecycle.js 77.77% <100%> (ø) ⬆️
src/packages/recompose/withState.js 88.88% <100%> (ø) ⬆️
src/packages/recompose/getContext.js 87.5% <100%> (ø) ⬆️
src/packages/recompose/nest.js 100% <100%> (ø) ⬆️
src/packages/recompose/withHandlers.js 93.75% <100%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5004622...56f83c3. Read the comment docs.

@istarkov
Copy link
Contributor

istarkov commented Oct 2, 2017

Super, wiil publish today-tomorrow!

@istarkov
Copy link
Contributor

istarkov commented Oct 2, 2017

First time in my life I see usage of react createFactory in a wild

@deepsweet
Copy link
Contributor Author

well I just tried to have the minimal diff :) we can go with JSX instead.

@istarkov
Copy link
Contributor

istarkov commented Oct 2, 2017

The minimal diff is fine for me, with JSX I think it will be a problems with preact etc

@wuct
Copy link
Contributor

wuct commented Oct 3, 2017

But there is no createFactory in preact, I think we should use JSX instead.

@istarkov
Copy link
Contributor

istarkov commented Oct 3, 2017

@wuct
Copy link
Contributor

wuct commented Oct 3, 2017

@istarkov Thanks! This PR looks good to me now :)

@istarkov
Copy link
Contributor

istarkov commented Oct 6, 2017

Thank you!!!!!

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 6, 2017

I'm curious about the motivation behind this change. Are you saying it's playing with fire forcing this performance optimisation, when instead, it could be something done by React internally?

My main frustration is that it runs ok in development while crushes on production.

Also, I never understood why this performance optimisation has been enable for production only at some point. Why not keeping it for all the env? Thanks for your 💡.

@istarkov
Copy link
Contributor

istarkov commented Oct 6, 2017

Initially all began here #442 (PR #473), at that time optimisation already worked differently at production and development (but for PropTypes case) and also was a point of problems with enzyme tests. But just that change wasn't the point of following problems ;-)
In that days we also merged this PR where instead of undocumented check is something is a React Component we started to check on render function existence on prototype. Since then our problems become more powerful - we found that not all the React Native components has render method in prototype, that some guys binds render method (so it does not exist in prototype) and all of them got exception only at production.
So the possible solutions was

  • make optimisation to work in dev also - that does't solve the error - people will get strange error in dev also ;-)
  • return previous check - breaking support of React-like frameworks and using undocumented (not sure it exists in react 16) method
  • found a better way to detect that something is Class Component and not the function (we didn't found it) (this solves almost all above problems but not all)
  • remove optimisation - as having that we can do it - why React itself can't do it also (I think React don;t do it before because it had functional components internally based on class components - but not sure I'm right here)

Having that react vdom is very fast - and having that at dev I see no performance problems last month+ (I know that it is very bad test - "I see no perf problem") we decided to remove it.
May be it will cause new discussion and proposals with new better solution ;-)

@istarkov
Copy link
Contributor

istarkov commented Oct 6, 2017

I've asked @acdlite here https://twitter.com/icelabaratory/status/916360249635999744
and will repeat here
@acdlite can you explain why React doesn't use eager optimisations like you did in recompose?

@istarkov
Copy link
Contributor

istarkov commented Oct 6, 2017

image

@oliviertassinari
Copy link
Contributor

@istarkov Thanks a lot for taking the time to answer the question in detail! Keep the good work.

@NMinhNguyen
Copy link

Shouldn’t this public API removal be considered a breaking change? Someone could be relying on these factories in their own HOC implementations.

@deepsweet
Copy link
Contributor Author

minor version bump while major is zero is already a breaking change according to semver.

@NMinhNguyen
Copy link

My bad, should’ve been clearer - perhaps having the word BREAKING in the release notes would’ve been clearer, but maybe it’s implicit from the minor version increase 😅

neighborhood999 added a commit to neighborhood999/recompose that referenced this pull request Oct 25, 2017
* export `createEventHandlerWithConfig()` and add documentation.

* Fix Prettier issues

* Docs: Fix missing comma in
 type description for `createEventHandlerWithConfig()`

* Docs: Add *WithConfig functions to TOC.

* Use $Compose instead Compose (acdlite#523)

* Use $Compose instead Compose

* Update flow-bin dependencies

* Allow React 16 (acdlite#530)

Resolves acdlite#506.

* recompose v0.25.1

* Updated base fiddle link (acdlite#534)

Base fiddle was using a unpkg link pointed at the old (React 15) dist url but without a React version, so broke with the release of React 16. Now points to the correct location for React 16, with 16 specified (shouldn't break this way again).

* Remove createEagerFactory and createEagerElement (acdlite#538)

* recompose v0.26.0

* Fix grammar in API.md (acdlite#542)

* Translate `createEventHandlerWithConfig()`
neighborhood999 added a commit to neighborhood999/recompose that referenced this pull request Nov 29, 2017
* export `createEventHandlerWithConfig()` and add documentation.

* Fix Prettier issues

* Docs: Fix missing comma in
 type description for `createEventHandlerWithConfig()`

* Docs: Add *WithConfig functions to TOC.

* Use $Compose instead Compose (acdlite#523)

* Use $Compose instead Compose

* Update flow-bin dependencies

* Allow React 16 (acdlite#530)

Resolves acdlite#506.

* recompose v0.25.1

* Updated base fiddle link (acdlite#534)

Base fiddle was using a unpkg link pointed at the old (React 15) dist url but without a React version, so broke with the release of React 16. Now points to the correct location for React 16, with 16 specified (shouldn't break this way again).

* Remove createEagerFactory and createEagerElement (acdlite#538)

* recompose v0.26.0

* Fix grammar in API.md (acdlite#542)

* Translate `createEventHandlerWithConfig()`
@kof
Copy link

kof commented Apr 17, 2018

@istarkov Was a different option for eager optimizations discussed? For e.g. only supporting functional components?

@gaearon
Copy link

gaearon commented Apr 17, 2018

return previous check - breaking support of React-like frameworks and using undocumented (not sure it exists in react 16) method

Looking for isReactComponent on the prototype is the supported way to check for whether something is a React component class. No, there are no plans to change it, and you can adopt the same check in your library. It’s not documented because it comes up very rarely and we don’t really want people writing apps thinking about this. But for library authors, it’s a good way to check.

Checking for render on the prototype is not, and the reasons you discovered later (with people’s code breaking in production) are exactly why we weren’t doing it in React either.

@developit @istarkov Please feel free to reach out to us next time you need to check something like this. We’re happy to explain motivations for why went for a particular check.

@istarkov
Copy link
Contributor

@kof the main issue was our inability to distinguish functional component from class component, both are functions (class is still a constructor function), both can have no render function in prototype, there were non documented method for React (now documented isReactComponent) but not for Preact.
Also even this optimization reduces virtual tree size there are no any performance tests showing that this is somehow seriously improves performance in real life. I haven't seen any drawbacks in our production or even development code.

@istarkov
Copy link
Contributor

And also this ;-)
image

@kof
Copy link

kof commented Apr 17, 2018

I haven't looked yet into each issue, but

different tree structure at dev and prod

yeah should not happen, this optimization should be always in place if at all

preact support

If it is about adding isReactComponent I am sure @developit will accept a pr for this.

@deepsweet
Copy link
Contributor Author

what would be (and it actually was) really misleading to many people is that you can't find something in React elements tree using Enzyme or similar tools because it was automagically "squashed" by quite internal and advanced condition check.

dev tools prettiness is doomed in any way because we still have stateful HOCs even if we can just call the rest as functions.

@kof
Copy link

kof commented Apr 17, 2018

What about a separate function which does the optimizations?

@deepsweet
Copy link
Contributor Author

it may play well at least for me if it's easy to alias it using some Babel plugin or Webpack only in production mode, just like react → preact seamless transition.

@gaearon
Copy link

gaearon commented Apr 17, 2018

Here's something I don't understand.

I agree that in general the edge cases of "executing" arbitrary user-provided components can be too confusing, and Recompose probably shouldn't attempt to do that by default.

But can't Recompose safely collapse its own components? E.g. mapProps(mapProps(mapProps(...))) should be fairly easy and safe to "collapse" in production—is it not?

@deepsweet
Copy link
Contributor Author

should be fairly easy and safe to "collapse" in production

and we made it so #473, but then it led to some other issues like #489 but I don't actually recall all the details 😳

@kof
Copy link

kof commented Apr 17, 2018

My main frustration is that it runs ok in development while crushes on production.

yeah, its dangerous to have different behavior in production, much better to stick with it everywhere.

@gaearon
Copy link

gaearon commented Apr 17, 2018

I think it should be safe to bring back if you do this only for Recompose components. Then you don't need flaky checks about whether something is a class or not—because you have full control over the components.

@deepsweet
Copy link
Contributor Author

deepsweet commented Apr 17, 2018

much better to stick with it everywhere

then as you already proposed it should be an explicit opt-in – this way user is more or less aware of what's going on.

if you do this only for Recompose components

I'm all for it if we have a proper way to do this – does Andrew's tweet mentioned above still make sense?

@gaearon
Copy link

gaearon commented Apr 17, 2018

does Andrew's tweet mentioned above still make sense?

I can't say if a tweet "makes sense". I'm not sure what you're asking. React won't do this optimization for now because it's not very useful to apply it everywhere. And it's something a compiler is better positioned to do.

But I'm not sure I'm seeing a problem with Recompose being a bit smarter around its own components. Maybe I'm missing something.

@istarkov
Copy link
Contributor

From my side I see nothing bad to accept PR which will not cause any issues we had before.

I wanna ask people how looks like your longest streak of composed recompose HOCs in current project. As having modern tools like relay, ready form state management libs like Formik, I almost lost the need to map-rename-with-and-do-something-with-Props in my projects.

As an example my current longest recompose streak looks like below (just parsed my code base)

withStateHandlers
withHandlers
withPropsOnChange
withProps <- only this is functional

What the performance benefit will be achieved on some kind of real example?
1% or more - hard to say without perf tests.

@gaearon
Copy link

gaearon commented Apr 17, 2018

I've seen some wild mapProps chaining in some projects.

@kof
Copy link

kof commented Apr 17, 2018

I landed in this discussion because I joined a project where I see an input field with 16 HOCs. I decided to dig into it because it didn't seem right. Now I am wrapping my had around why that happens and where it needs to be fixed. Right now it seems it is both - recompose and our code.

In general as an architect I am looking for a way to guide other developers on how to write code without landing in this mess of 16 HOCs input.

@deepsweet
Copy link
Contributor Author

from what I got from that tweet is that isReferentiallyTransparentFunctionComponent was not enough and kinda naive. I might be wrong interpreting it :)

@gaearon
Copy link

gaearon commented Apr 17, 2018

We chatted with @acdlite today and agreed this might be a reasonable path:

  • only "collapse" known Recompose components
  • do this both in development and production

@kof
Copy link

kof commented Apr 17, 2018

So we have a way to identify a react class based component, but we have no way to identify a functional one.

What if we could use for e.g. Component constructor as a factory to create a component from a function?

import {Component} from 'react'
const Button = Component((props) => <button></button>)

Button.isFunctionalComponent // true

Now we could do optimizations with user components as well, right

@gaearon
Copy link

gaearon commented Apr 17, 2018

Why do you need to identify them? Recompose components could be tagged with its own flag which Recompose components would check.

@gaearon
Copy link

gaearon commented Apr 17, 2018

Oh I missed the last sentence. I don’t think this makes a lot of sense for a few reasons. Feel free to raise a suggestion on React RFC repo but I don’t see people rewriting all components like this. In any case this seems unrelated to the discussion.

@kof
Copy link

kof commented Apr 18, 2018

I don’t see people rewriting all components like this.

Me neither unless there are more reasons to be able to detect a functional component. For now it could be just recompose. Recompose could do heavy lifting for user components if it knows they are functional components.

@deepsweet
Copy link
Contributor Author

I'd vote for special static property to be able to mark any other "3rd party" HOC with it. at the same time it feels like it should be kinda internal Recompose thing exposed only for HOCs authors, at least at this point and mostly to not create unnecessary confusion.

@gaearon
Copy link

gaearon commented Apr 18, 2018

I suggest not trying to over-abstract and focusing on this specific problem which should be soluble.

@deepsweet
Copy link
Contributor Author

only "collapse" known Recompose components

Why do you need to identify them? Recompose components could be tagged with its own flag

So are we talking about a special static property? I'd really like to allow 3rd party HOCs passed to compose to be "squashed" as well.

isFunctionalHOC?

@deepsweet
Copy link
Contributor Author

So far my idea is something like this:

import { createElement } from 'react'

export const isFunctionalHocSymbol = 'IS_FUNCTIONAL_HOC_OR_BETTER_NAME'

export const createEagierElement = (type, props, ...children) => {
  if (typeof type === 'function' && type[isFunctionalHocSymbol] === true) {
    if (children.length === 0) {
      return type(props)
    }

    if (children.length === 1) {
      return type({ ...props, children: children[0] })
    }

    return type({ ...props, children })
  }

  return createElement(type, props, ...children)
}

Probably we can even omit typeof type === 'function' && part.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.25.0 update] createEagerFactory production only error
8 participants