feat(ElementType): new wrapper for all elements #2306

layershifter · 2017-11-09T20:45:34Z

Replaces #1879.

What?

This PR offers the complex multipurpose solution for our internal problems with refs. These changes will allow to improve work with as prop and refs.

Why so complex?

We cannot simply use ref, it' isn't a prop. It has a same behaviour as key - it never passed to a component. However, there were numerous issues about refs (#1740, #1602, #1849, #1732 and more) and it's time to make something in this way. document.querySelector is a workaround, but it's not a solution.

Problem is a well known in React's world, there is a long-live issue facebook/react#4213, there was also the RFC facebook/react#6974 from Dan.

Variants

Project	Prop	Example
formsy-react-components	`componentRef` & `innerRef`	`refs.md`
office-ui-fabric-react	`componentRef`	office-ui-fabric-react#1356
react-redux	`connect` HOC, `withRef`	reduxjs/react-redux@`2d3d0be`
react-router	`innerRef`	`Link.md`

Solution

Several months ago ☹️ I've started to work on this problem. I made few iterations and checked many solutions. This PR is its result, the solution consists from two components.

`withComputedType`

This is a HOC that allows to pass an already computed element type in as prop. In fact it's the most controversial part of this PR.

We need the computed type only in several components (i.e. Button, Image), it also simplifies work with default as values (see Form, Image). I think that we can go with it, as we plan more HOCs in future for transitions and popups.

The only valid against that I see is the increase of the render tree, all our components will be wrapped with it.

`ElementType`

It has the only one task, to decide how to handle innerRefs. If we simply wrap it with Ref we will always call findDOMNode, even when we actually don't need it.

P.S.

I want to ship this PR as start point and apply changes to left components iteratively. After all components will be updated, I will able to cleanup tests.

layershifter · 2017-11-12T19:50:22Z

src/elements/Container/Container.js

 } from '../../lib'

 /**
 * A container limits content to a maximum width.
 */
-function Container(props) {
+const InnerContainer = (props) => {


As Container is wrapped with HOC, we can't user prototype.name. So, we can use arrow functions for components

_meta should be exported on the default export, now it's an enhanced component. However, I'm not happy about Inner- prefix

layershifter · 2017-11-12T19:51:05Z

src/elements/Image/Image.js

+  if (!_.isNil(dimmer) || !_.isNil(label) || !_.isNil(wrapped) || !childrenUtils.isNil(children)) return 'div'
+}
+
+const Image = withComputedType(computeType)(InnerImage)


Type computing function moved to HOC

layershifter · 2017-11-12T19:53:40Z

src/elements/List/ListHeader.js

  const { children, className, content } = props
  const classes = cx('header', className)
-  const rest = getUnhandledProps(ListHeader, props)
-  const ElementType = getElementType(ListHeader, props)
+  const rest = getUnhandledProps(InnerListHeader, props, { passAs: true })


Another nasty thing. as should be passed to ElementType, but not all our components are updated.

I've already add ignoreProps option babel-plugin-transform-react-handled-props plugin, but we can't enable it until we will update all components

@layershifter do we have a list of the components that still need to get updated somewhere? I am assuming this exists in another conversation/PR somewhere.

After this PR will be merged I will update #1321 with list for update. I don't want to do this in single PR because there will be too many changes.

layershifter · 2017-11-12T19:54:18Z

src/lib/getUnhandledProps.js

 * @returns {{}} A shallow copy of the prop object
 */
-const getUnhandledProps = (Component, props) => {
+const getUnhandledProps = (Component, props, options = {}) => {


This function will be restored after we will update all components.

codecov-io · 2017-11-12T20:10:44Z

Codecov Report

Merging #2306 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2306      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         160      160              
  Lines        2751     2767      +16     
==========================================
+ Hits         2744     2760      +16     
  Misses          7        7

Impacted Files	Coverage Δ
src/views/Item/ItemImage.js	`100% <ø> (ø)`	⬆️
src/elements/List/ListHeader.js	`100% <100%> (ø)`	⬆️
src/elements/Container/Container.js	`100% <100%> (ø)`	⬆️
src/elements/List/ListDescription.js	`100% <100%> (ø)`	⬆️
src/elements/List/ListItem.js	`100% <100%> (ø)`	⬆️
src/elements/List/ListContent.js	`100% <100%> (ø)`	⬆️
src/elements/Image/Image.js	`100% <100%> (ø)`	⬆️
src/views/Advertisement/Advertisement.js	`100% <100%> (ø)`	⬆️
src/elements/List/ListIcon.js	`100% <100%> (ø)`	⬆️
src/elements/List/List.js	`100% <100%> (ø)`	⬆️
... and 1 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 d285ff2...a6b9ac1. Read the comment docs.

layershifter · 2017-11-12T20:12:09Z

@levithomason I'll be glad to hear your feedback 😄

levithomason · 2017-11-25T17:44:48Z

So sorry for the delay, I have not had enough time to fully grok this one. Any input by @davezuko or @brianespinosa would be much help.

I don't want to block progress, but we also need to validate this approach across another min or two.

…/Semantic-Org/Semantic-UI-React into feat/element-type # Conflicts: # src/elements/Image/Image.js # test/specs/elements/Image/Image-test.js

Nickersoft · 2018-01-11T17:47:28Z

Any update on the status of this?

levithomason · 2018-03-27T01:25:36Z

I'm worried about the complexity here. My primary concern is this obfuscates the basic definition what a component is and does so for all of our components.

Thankfully, it looks like React will now support ref forwarding which may solve this problem for us with React.forwardRef, see reactjs/rfcs#30.

Per @mxstbr's approval of that PR:

This would work 100% for styled-components. innerRef is one of the uglier patches of our API and I'd love to get rid of it and make the wrapper components truly transparent and pass-through.

I would agree with this sentiment. I want to try some other approaches before merging this. I'd also be open to leaving this unsolved until React >= 16.3 ships (with React.forwardRef) and requiring that.

brianespinosa · 2018-03-27T17:03:00Z

I'd also be open to leaving this unsolved until React >= 16.3 ships (with React.forwardRef) and requiring that.

Agreed. Seems to be close (relatively speaking). If ref forwarding will solve our issue it probably makes the most sense to wait for that.

levithomason · 2018-05-01T00:03:25Z

Hear me out before throwing stones but I'm beginning to consider extending a base component for all our HOC needs. Why?!

I think a component library is a unique thing in terms of React components for apps. In an application, you have many types of components with varying needs. Whereas, in a component library, every component needs identical base functionality.

We can either bundle this into a single base component that we extend, or, go the functional approach of making several HOCs and wrap every component with every HOC to get the base feature set needed.

HOCs are hard to debug, test through, and add a lot to the render tree. Again, in an application, it is typically easier to test dumb components and keep more complex features in a single tested HOC. However, in a component library, it seems ludicrous to have every component be a stack of function calls and component wrappers. Every element in a consumer's app would be a stack of function calls and its own notable render tree. Imagine when they build more complex controls, or, imagine our own controls such as an Input that contains a Button and a Label. Rather than rendering three React components, we'd be rendering n*h components for a single control where n is the number of components and h is the number of HOCs required for our base functionality.

A base component would make every component in the library a single flat component. It would also very easily mean all components have all the features they need.

I know this is not the direction of the community, but, I'd love to hear thoughts and opinions on this.

brianespinosa · 2018-05-01T03:24:35Z

@levithomason I can also see how this simplifies things with Fela styling too. Additionally, my OCD hates the deep render tree.

levithomason · 2018-05-01T05:07:44Z

I'll try updating a few components to use a base component and see how it goes.

layershifter · 2018-05-01T06:01:47Z

@levithomason I'm think that we should go with forwardRef, I will check it and try to use with SUIR.

levithomason · 2018-05-02T05:57:04Z

OK, I'll wait for you. Are you thinking it will solve this problem?:

const StatelessComponent = () => <div />

<Button as={StatelessComponent} />

Warning: Stateless function components cannot be given refs.

levithomason · 2018-05-06T15:31:07Z

Adding this to #2747

layershifter · 2018-05-26T08:23:18Z

Closing in favor of #2844.

layershifter mentioned this pull request Nov 10, 2017

Highlight in the Documentation: Dropdown requires className='link item' #2304

Closed

layershifter force-pushed the feat/element-type branch from 5633ae5 to d9f1353 Compare November 12, 2017 19:47

layershifter commented Nov 12, 2017

View reviewed changes

feat(ElementType): new wrapper for all elements

d117cf6

layershifter force-pushed the feat/element-type branch from d9f1353 to d117cf6 Compare November 12, 2017 20:03

layershifter added the ready for review label Nov 12, 2017

layershifter mentioned this pull request Dec 6, 2017

RFC: Be able to define fallback order for Popup position #2359

Closed

layershifter added 2 commits December 17, 2017 09:30

Merge branches 'feat/element-type' and 'master' of https://github.com…

3c6adc4

…/Semantic-Org/Semantic-UI-React into feat/element-type # Conflicts: # src/elements/Image/Image.js # test/specs/elements/Image/Image-test.js

fix(Image): fix merge problems

93cde4d

layershifter mentioned this pull request Jan 8, 2018

ref error when using "as" prop in <Visibility /> component #2412

Closed

brianespinosa mentioned this pull request Feb 20, 2018

Dealing with Button as a NavLink of react router gives a react warning #2295

Closed

levithomason mentioned this pull request Feb 26, 2018

Support transition and popup props #2552

Open

layershifter mentioned this pull request Mar 11, 2018

fix(Modal): fix positioning when "as" prop is a component #2623

Merged

fix(isConformant): merge master

a6b9ac1

levithomason mentioned this pull request Apr 24, 2018

Modal cannot be given "as". #2731

Closed

levithomason added the needs engineering label May 2, 2018

levithomason removed the ready for review label May 2, 2018

levithomason mentioned this pull request May 6, 2018

Update to React 16 #2747

Closed

5 tasks

layershifter mentioned this pull request May 9, 2018

Error received when setting Button 'as' to a SFC #2779

Closed

layershifter mentioned this pull request May 26, 2018

feat(forwardRef): add forwardRefFactory #2844

Closed

layershifter closed this May 26, 2018

levithomason deleted the feat/element-type branch July 22, 2018 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ElementType): new wrapper for all elements #2306

feat(ElementType): new wrapper for all elements #2306

layershifter commented Nov 9, 2017 •

edited

Loading

layershifter Nov 12, 2017

layershifter Nov 12, 2017

layershifter Nov 12, 2017

brianespinosa Nov 27, 2017

layershifter Nov 27, 2017

layershifter Nov 12, 2017

codecov-io commented Nov 12, 2017 •

edited

Loading

layershifter commented Nov 12, 2017

levithomason commented Nov 25, 2017

Nickersoft commented Jan 11, 2018

levithomason commented Mar 27, 2018

brianespinosa commented Mar 27, 2018

levithomason commented May 1, 2018

brianespinosa commented May 1, 2018

levithomason commented May 1, 2018

layershifter commented May 1, 2018

levithomason commented May 2, 2018

levithomason commented May 6, 2018

layershifter commented May 26, 2018

feat(ElementType): new wrapper for all elements #2306

feat(ElementType): new wrapper for all elements #2306

Conversation

layershifter commented Nov 9, 2017 • edited Loading

What?

Why so complex?

Variants

Solution

withComputedType

ElementType

P.S.

layershifter Nov 12, 2017

Choose a reason for hiding this comment

layershifter Nov 12, 2017

Choose a reason for hiding this comment

layershifter Nov 12, 2017

Choose a reason for hiding this comment

brianespinosa Nov 27, 2017

Choose a reason for hiding this comment

layershifter Nov 27, 2017

Choose a reason for hiding this comment

layershifter Nov 12, 2017

Choose a reason for hiding this comment

codecov-io commented Nov 12, 2017 • edited Loading

Codecov Report

layershifter commented Nov 12, 2017

levithomason commented Nov 25, 2017

Nickersoft commented Jan 11, 2018

levithomason commented Mar 27, 2018

brianespinosa commented Mar 27, 2018

levithomason commented May 1, 2018

brianespinosa commented May 1, 2018

levithomason commented May 1, 2018

layershifter commented May 1, 2018

levithomason commented May 2, 2018

levithomason commented May 6, 2018

layershifter commented May 26, 2018

layershifter commented Nov 9, 2017 •

edited

Loading

`withComputedType`

`ElementType`

codecov-io commented Nov 12, 2017 •

edited

Loading