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

Make renderNothing as a classComponent #289

Merged
merged 2 commits into from Dec 20, 2016
Merged

Conversation

istarkov
Copy link
Contributor

@istarkov istarkov commented Dec 12, 2016

Fixes #288

The problem that renderNothing mostly used with branch component,
and since in release 0.21.0 branch does not create a class,
we have that functional component returns null, what is prohibited with React 0.14.x and only possible with https://github.com/facebook/react/releases/tag/v15.0.0

Also as renderNothing is always a terminator so it should not cause perf issues.

PS: not possible to write test, I have no knowledge how to have few react versions simultaneously

@istarkov
Copy link
Contributor Author

@wuct what are you think about?

t.is(nothing.displayName, 'Nothing')
const Nothing = renderNothing('div')
const wrapper = shallow(<Nothing />)
t.is(wrapper.type(), null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enzyme way to check that Component renders null docs

@wuct
Copy link
Contributor

wuct commented Dec 13, 2016

Maybe we can write a script to run tests with different version of React. Something like:

npm i react@15
npm test
npm i react@0.14.x
npm test

class Nothing extends Component {
render() {
return null
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the class component only in react@0.14:

import { Component, version } from 'react'

if (version.startsWith('0.14')) {
  class Nothing extends Component {
    render() {
      return null
    }
  }
} else {
  const Nothing = () => null
}

The unused part of code will be removed in production. @istarkov how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I know big developer daddies always try to avoid third party libraries version check like if (version.startsWith('0.14')) { ;-)

Why do you think that unused part will be removed at production?
My usual build tools babel + webpack + UglifyJsPlugin does not remove anything in such case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap, you're right. UglifyJs or other JS minifiers do not remove it. I have to admit that I haven't done this before. What I said is just from my limited knowledge :)

@wuct wuct self-assigned this Dec 14, 2016
@wuct
Copy link
Contributor

wuct commented Dec 14, 2016

@istarkov Do you think we need to test it against react 0.14? If so, we can add the script to here to run all tests twice. Beside that, these PR looks good to me.

@istarkov
Copy link
Contributor Author

I think no need to test in react 14 as class component which returns null in it's render method is allowed in both.

@istarkov istarkov merged commit 9af9095 into master Dec 20, 2016
@istarkov istarkov deleted the fix-render-nothing-react-14 branch December 20, 2016 18:13
@istarkov
Copy link
Contributor Author

And now my work codebase works as expected! ;-)

@wuct
Copy link
Contributor

wuct commented Dec 21, 2016

Nice!

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Jan 9, 2017

Notice that this change is a breaking change for us. We were relying on the referential transparency feature of the renderNothing Higer-order Component to do a branch logic.

We reverted back to the old version:

import createHelper from 'recompose/createHelper';

const renderNothing = () => {
  const Nothing = () => null;
  Nothing.displayName = 'Nothing';

  return Nothing;
};

export default createHelper(renderNothing, 'renderNothing', false, true);

@istarkov
Copy link
Contributor Author

istarkov commented Jan 9, 2017

What has changed for you? What means relying on referential transparency? What the problem if branch will create element and not just call a function?

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Jan 9, 2017

@istarkov The change of behavior is simply coming from the referential transparency change.
Here is a reproduction example.

const React = require('react');
const { renderToString } = require('react-dom/server');
const recompose = require('recompose');

const renderNothing = () => () => null;

const Component = recompose.compose(
  recompose.branch(
    ({ children }) => !children, // was null
    () => () => <div>a</div>,
    () => () => <div>b</div>,
  ),
)(() => null);

console.log(renderToString(
  <Component>
    {recompose.createEagerElement(renderNothing())}
  </Component>
));
// <div data-reactroot="" data-reactid="1" data-react-checksum="-2026303557">a</div>

console.log(renderToString(
  <Component>
    {recompose.createEagerElement(recompose.renderNothing())}
  </Component>
));
// <div data-reactroot="" data-reactid="1" data-react-checksum="-2025844804">b</div>

@istarkov
Copy link
Contributor Author

istarkov commented Jan 9, 2017

Got it, thank you, interesting behavior when referential transparancy can affect rendering.

@istarkov
Copy link
Contributor Author

I like the Idea when render nothing renders really nothing, but have no Idea now how to make it work with React 0.14.x, any ideas?

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Jan 10, 2017

any ideas?

No clue, sorry. Why do you want to support react@0.14.x in the first place?

@istarkov
Copy link
Contributor Author

Why do you want to support react@0.14.x in the first place?

I have no strong decision what versions of React we should support,

For example I still use React 0.14 in a big project and will move 2 newer React not faster than 2-3 month. (It is because of wrong decisions I made, we use some third party libraries in a hackery way, so can't move fast on newer versions of that libraries, and our versions of that libraries are incompatible with React 0.15)

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.

None yet

3 participants