Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Disregard falsy elements when walking tree #1495

Merged
merged 5 commits into from
Dec 29, 2017
Merged

Disregard falsy elements when walking tree #1495

merged 5 commits into from
Dec 29, 2017

Conversation

bostrom
Copy link
Contributor

@bostrom bostrom commented Dec 29, 2017

This PR fixes a problem with server side rendering when rendering an array and one or more of the items in the array is falsy (but other than null). This would throw an cannot read property type of null error.

An example case when this can happen is when you do a inline conditional render:

const renderSmth = false;

render() {
  return [
    renderSmth && (<Smth key="smth" />),
    <MoreContent key="morecontent" />
  ];
}

React handles this case gracefully by disregarding falsy values, not rendering them. I would like to assume that Apollo would do the same, but that's not the case, currently it only checks if an element is null.

This PR replaces the element == null with !element, effectively disregarding all falsy values altogether, more closely mimicking the behaviour of React.

@apollo-cla
Copy link

@bostrom: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@rosskevin rosskevin merged commit c95f7cb into apollographql:master Dec 29, 2017
@rosskevin
Copy link
Contributor

Thanks @bostrom

@bostrom bostrom deleted the ssr-disregard-falsy branch December 29, 2017 15:49
@TzviPM
Copy link

TzviPM commented Jan 2, 2018

@rosskevin any idea when the next react-apollo release will be, so we can take advantage of this fix?

@rosskevin
Copy link
Contributor

@Omniroot We don't have a timeline yet, but if you are up for using master, feel free to grab my package for the time being (you'll have to either alias your imports or search/replace for the time being). https://www.npmjs.com/package/@alienfast/react-apollo

We will be discussing strategy for the next release on Friday 1/5/18 with the committers.

@bostrom
Copy link
Contributor Author

bostrom commented Jan 3, 2018

@Omniroot as a workaround, you can filter the array:

render() {
  return [
    renderSmth && (<Smth key="smth" />),
    <MoreContent key="morecontent" />
  ].filter(Boolean);
}

@TzviPM
Copy link

TzviPM commented Jan 8, 2018

@rosskevin any idea what the plan is for the next release?

@rosskevin
Copy link
Contributor

@Omniroot we are discussing on a call tomorrow, call was moved from Friday.

@TzviPM
Copy link

TzviPM commented Jan 15, 2018

@rosskevin any updates?

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

4 participants