Relay: Relay 1.6 changed fragment container rendering logic

[steida]

This is probably related to recent async changes. With Relay 1.5, both AttendanceFilter and Attendance were rerendered when Index was rerendered.

With Relay 1.6, components wrapped by createFragmentContainer are not updated. Not sure whether it's good or bad behavior, but it is a breaking change. As a workaround, when I add key={Math.random{}}, wrapped components are properly rerendered.

// @flow
import * as React from 'react';
import Page from '../components/core/Page';
import app from '../components/app';
import { titles } from '../components/app/sitemap';
import AttendanceFilter from '../components/AttendanceFilter';
import { graphql } from 'react-relay';
import Attendance from '../components/Attendance';
import Block from '../components/core/Block';
import Row from '../components/core/Row';

const Index = ({ data }) => {
  return (
    <Page title={intl => intl.formatMessage(titles.index)}>
      <Block>
        <Row>
          <AttendanceFilter data={data} />
        </Row>
      </Block>
      <Attendance data={data} />
    </Page>
  );
};

export default app(Index, {
  query: graphql`
    query pagesQuery($month: String, $year: String) {
      ...AttendanceFilter
      ...Attendance @arguments(month: $month, year: $year)
    }
  `,
});

[jstejada]

Hey @steida, that's no good! Can you show us what those fragment containers look like internally?

[steida]

// @flow
import * as React from 'react';
import Row from './core/Row';
import { withRouter } from 'next/router';
import MonthPicker from './MonthPicker';
import YearPicker from './YearPicker';
import WorkerPicker from './WorkerPicker';
import { createFragmentContainer, graphql } from 'react-relay';
import * as generated from './__generated__/AttendanceFilter.graphql';

type QueryKey = 'worker' | 'month' | 'year';

export type AttendanceFilterUrlQuery = { [QueryKey]: ?string };

type AttendanceFilterProps = {
  data: generated.AttendanceFilter,
  router: {
    push: Object => void,
    pathname: string,
    query: AttendanceFilterUrlQuery,
  },
};

class AttendanceFilter extends React.PureComponent<AttendanceFilterProps> {
  onPickerValueChange = (queryKey: QueryKey) => (value: string) => {
    const { router } = this.props;
    const query = { ...router.query };
    const isEmpty = value.length === 0;
    if (isEmpty) delete query[queryKey];
    router.push({
      pathname: router.pathname,
      query: {
        ...query,
        ...(!isEmpty ? { [queryKey]: value } : null),
      },
    });
  };

  onWorkerPickerValueChange = this.onPickerValueChange('worker');
  onMonthPickerValueChange = this.onPickerValueChange('month');
  onYearPickerValueChange = this.onPickerValueChange('year');

  render() {
    const {
      data,
      router: { query },
    } = this.props;

    return (
      <Row>
        {/* $FlowFixMe https://github.com/facebook/relay/issues/2316 */}
        <WorkerPicker
          data={data}
          selectedValue={query.worker}
          onValueChange={this.onWorkerPickerValueChange}
        />
        {/* $FlowFixMe https://github.com/facebook/relay/issues/2316 */}
        <MonthPicker
          data={data}
          selectedValue={query.month || ''}
          onValueChange={this.onMonthPickerValueChange}
        />
        {/* $FlowFixMe https://github.com/facebook/relay/issues/2316 */}
        <YearPicker
          data={data}
          selectedValue={query.year || ''}
          onValueChange={this.onYearPickerValueChange}
        />
      </Row>
    );
  }
}

export default createFragmentContainer(
  withRouter(AttendanceFilter),
  graphql`
    fragment AttendanceFilter on Query {
      ...WorkerPicker
      ...MonthPicker
      ...YearPicker
    }
  `,
);

[steida]

// @flow
import * as React from 'react';
import { FormattedDate } from 'react-intl';
import { View } from 'react-native';
import Picker from './core/Picker';
import TextInput from './core/TextInput';
import Text from './core/Text';
import { withRouter } from 'next/router';
import { createFragmentContainer, graphql } from 'react-relay';
import * as generated from './__generated__/Attendance.graphql';
import Theme from './core/Theme';
import Block from './core/Block';
import Row from './core/Row';
import { SaveButton, CancelButton } from './core/buttons';
import { update, insert } from 'ramda';
import Mutation from './core/Mutation';
import SaveAttendanceMutation from '../mutations/SaveAttendanceMutation';
import Button from './core/Button';
import type { AttendanceFilterUrlQuery } from './AttendanceFilter';

type PassagePickerProps = {|
  data: *,
  selectedValue: *,
  onValueChange: *,
  disabled: *,
|};

class PassagePicker extends React.PureComponent<PassagePickerProps> {
  handleValueChange = value => {
    // Prefix is must, because ID must be unique in Relay.
    const valueWithoutPrefix = value.replace('passage', '') || null;
    this.props.onValueChange(valueWithoutPrefix);
  };

  render() {
    const { data, selectedValue, disabled } = this.props;
    return (
      <Picker
        selectedValue={`passage${selectedValue || ''}`}
        onValueChange={this.handleValueChange}
        disabled={disabled}
      >
        <Picker.Item label="--" value="passage" />
        {data.map(item => (
          <Picker.Item label={item.text} value={item.id} key={item.id} />
        ))}
      </Picker>
    );
  }
}

const AttendanceRow = ({ col1, col2, col3, col4, col5, col6, col7 }) => (
  <Theme>
    {({ styles }) => (
      <View style={styles.attendanceRow}>
        <View style={styles.attendanceButtonColumn}>{col1}</View>
        <View style={styles.attendanceDateColumn}>{col2}</View>
        {/* <View style={{ flexWrap: 'nowrap' }}> */}
        <View style={styles.attendanceTimeColumn}>{col3}</View>
        <View style={styles.attendancePickerColumn}>{col4}</View>
        {/* </View> */}
        {/* <View style={{ flexWrap: 'nowrap' }}> */}
        <View style={styles.attendanceTimeColumn}>{col5}</View>
        <View style={styles.attendancePickerColumn}>{col6}</View>
        {/* </View> */}
        <View>{col7}</View>
      </View>
    )}
  </Theme>
);

const noop = () => {};

const AttendanceButtons = ({ pending, isDirty, onCancel, onSave }) => (
  <Block>
    <Row>
      {isDirty ? (
        <SaveButton
          color="primary"
          onPress={onSave}
          inline
          disabled={!isDirty || pending}
        />
      ) : (
        <Button inline onPress={noop} disabled>
          Saved
        </Button>
      )}
      {isDirty && (
        <CancelButton
          color="warning"
          onPress={onCancel}
          inline
          disabled={pending}
        />
      )}
    </Row>
  </Block>
);

class AttendanceHeader extends React.PureComponent<{}> {
  render() {
    return (
      <AttendanceRow
        col1={null}
        col2={<Text bold>Datum</Text>}
        col3={<Text bold>Příchod</Text>}
        col4={<Text bold>Typ příchodu</Text>}
        col5={<Text bold>Odchod</Text>}
        col6={<Text bold>Typ odchodu</Text>}
        col7={<Text bold>Text svátku</Text>}
      />
    );
  }
}

type TimeInputProps = {|
  onChangeText: *,
  value: *,
  disabled: *,
|};

type TimeInputState = {|
  isValid: boolean,
|};

class TimeInput extends React.PureComponent<TimeInputProps, TimeInputState> {
  static isValid = value => /^[0-9][0-9]:[0-9][0-9]:[0-9][0-9]$/.test(value);

  constructor(props) {
    super(props);
    this.state = {
      isValid: TimeInput.isValid(props.value),
    };
  }

  componentWillReceiveProps(nextProps) {
    this.setState({
      isValid: TimeInput.isValid(nextProps.value),
    });
  }

  render() {
    return (
      <TextInput
        style={!this.state.isValid && { color: 'red' }}
        onChangeText={this.props.onChangeText}
        value={this.props.value}
        disabled={this.props.disabled}
      />
    );
  }
}

type AttendanceItemProps = {|
  passagePicker: *,
  index: *,
  item: *,
  onChange: *,
  onAdd: *,
  onDelete: *,
  pending: *,
  firstInDate: *,
|};

class AttendanceItem extends React.PureComponent<AttendanceItemProps> {
  // It's used for all browsers to reveal potential problem asap.
  static fixIE11FormattedDate = message =>
    message
      // Remove IE11 superfluous whitespaces.
      // https://github.com/yahoo/react-intl/issues/201#issuecomment-153525729
      .replace(/\u200E/g, '')
      // Replace 1 2 with 1/2
      .replace(/(\d+)\s(\d+)/g, '$1/$2');

  onAdd = () => {
    this.props.onAdd(this.props.index);
  };

  onDelete = () => {
    this.props.onDelete(this.props.index);
  };

  createHandler = prop => text => {
    this.props.onChange(this.props.index, {
      ...this.props.item,
      [prop]: text,
    });
  };

  handleInputTimeChange = this.createHandler('inputTime');
  handleInputEventTypeChange = this.createHandler('inputEventType');
  handleOutputTimeChange = this.createHandler('outputTime');
  handleOutputEventTypeChange = this.createHandler('outputEventType');

  render() {
    const { item, pending, firstInDate } = this.props;
    const isToday =
      new Date(item.date).toDateString() === new Date().toDateString();
    return (
      <AttendanceRow
        col1={
          firstInDate ? (
            <Button color="primary" inline onPress={this.onAdd}>
              +
            </Button>
          ) : (
            <Button color="warning" inline onPress={this.onDelete}>
              -
            </Button>
          )
        }
        col2={
          <Text color={isToday ? 'success' : 'black'}>
            <FormattedDate
              value={item.date}
              weekday="short"
              day="numeric"
              month="numeric"
            >
              {message => AttendanceItem.fixIE11FormattedDate(message)}
            </FormattedDate>
          </Text>
        }
        col3={
          <TimeInput
            onChangeText={this.handleInputTimeChange}
            value={item.inputTime}
            disabled={pending}
          />
        }
        col4={
          <PassagePicker
            data={this.props.passagePicker}
            selectedValue={item.inputEventType}
            onValueChange={this.handleInputEventTypeChange}
            disabled={pending}
          />
        }
        col5={
          <TimeInput
            onChangeText={this.handleOutputTimeChange}
            value={item.outputTime}
            disabled={pending}
          />
        }
        col6={
          <PassagePicker
            data={this.props.passagePicker}
            selectedValue={item.outputEventType}
            onValueChange={this.handleOutputEventTypeChange}
            disabled={pending}
          />
        }
        col7={<Text>{item.holiday}</Text>}
      />
    );
  }
}

type AttendanceProps = {|
  data: generated.Attendance,
  router: { query: AttendanceFilterUrlQuery },
|};

type AttendanceState = {|
  attendance: $PropertyType<generated.Attendance, 'attendance'>,
|};

class Attendance extends React.PureComponent<AttendanceProps, AttendanceState> {
  static isDirty(attendance, savedAttendance) {
    // JSON.stringify is fast enough, 0.1ms average.
    return JSON.stringify(attendance) !== JSON.stringify(savedAttendance);
  }

  constructor(props: AttendanceProps) {
    super(props);
    this.state = {
      attendance: props.data.attendance,
    };
  }

  // TODO: Use getDerivedStateFromProps.
  // https://github.com/zeit/next.js/issues/4188
  componentWillReceiveProps(nextProps) {
    this.setState({
      attendance: nextProps.data.attendance,
    });
  }

  handleItemChange = (index, item) => {
    this.setState(({ attendance }) => {
      if (attendance == null) return;
      return {
        // $FlowFixMe Ramda libdef
        attendance: update(index, item, attendance),
      };
    });
  };

  handleItemAdd = index => {
    this.setState(({ attendance }) => {
      if (attendance == null) return;
      const item = {
        ...attendance[index],
        inputEventType: null,
        inputId: '0',
        inputTime: '00:00:00',
        outputEventType: null,
        outputId: '0',
        outputTime: '00:00:00',
      };
      let nextInDateIndex = index + 1;
      while (
        attendance[nextInDateIndex] != null &&
        attendance[nextInDateIndex].date === item.date
      ) {
        nextInDateIndex++;
      }
      return {
        // $FlowFixMe Ramda libdef
        attendance: insert(nextInDateIndex, item, attendance),
      };
    });
  };

  handleItemDelete = index => {
    this.setState(({ attendance }) => {
      if (attendance == null) return;
      const item = {
        ...attendance[index],
        deleted: true,
      };
      return {
        // $FlowFixMe Ramda libdef
        attendance: update(index, item, attendance),
      };
    });
  };

  handleCancel = () => {
    this.setState({ attendance: this.props.data.attendance });
  };

  handleSave = mutate => () => {
    const { attendance } = this.state;
    if (attendance == null) return;
    const variables = {
      attendance: attendance.map(item => {
        const withoutHoliday = { ...item };
        delete withoutHoliday.holiday;
        return withoutHoliday;
      }),
      month: this.props.router.query.month,
      year: this.props.router.query.year,
    };
    mutate(SaveAttendanceMutation.commit, variables);
  };

  render() {
    const { passagePicker } = this.props.data;
    const { attendance } = this.state;
    const savedAttendance = this.props.data.attendance;
    if (savedAttendance == null || attendance == null || passagePicker == null)
      return null;
    const isDirty = Attendance.isDirty(attendance, savedAttendance);
    let currentDate = null;
    let inDateIndex = 0;

    return (
      <Theme>
        {theme => (
          <Mutation>
            {({ mutate, pending }) => (
              <View>
                <AttendanceButtons
                  pending={pending}
                  isDirty={isDirty}
                  onCancel={this.handleCancel}
                  onSave={this.handleSave(mutate)}
                />
                <AttendanceHeader />
                <View style={theme.styles.attendanceTable}>
                  {attendance.map((item, index) => {
                    if (currentDate !== item.date) {
                      currentDate = item.date;
                      inDateIndex = 0;
                    } else {
                      inDateIndex++;
                    }
                    if (item.deleted) return null;
                    return (
                      <AttendanceItem
                        passagePicker={passagePicker}
                        // Attendance is a structure without unique ID.
                        key={`${item.date}-${inDateIndex}`}
                        index={index}
                        item={item}
                        onChange={this.handleItemChange}
                        onAdd={this.handleItemAdd}
                        onDelete={this.handleItemDelete}
                        pending={pending}
                        firstInDate={inDateIndex === 0}
                      />
                    );
                  })}
                </View>
                <AttendanceButtons
                  pending={pending}
                  isDirty={isDirty}
                  onCancel={this.handleCancel}
                  onSave={this.handleSave(mutate)}
                />
              </View>
            )}
          </Mutation>
        )}
      </Theme>
    );
  }
}

export default createFragmentContainer(
  withRouter(Attendance),
  graphql`
    fragment Attendance on Query
      @argumentDefinitions(
        month: { type: "String" }
        year: { type: "String" }
      ) {
      attendance(month: $month, year: $year) {
        date
        inputId
        inputTime
        inputEventType
        outputId
        outputTime
        outputEventType
        colorId
        holiday
      }
      passagePicker {
        id
        text
      }
    }
  `,
);

[steida]

withRouter does not do that. No log fok on rerender.

const Test = () => {
  console.log('fok');
  return null;
};

export default createFragmentContainer(
  Test,
  graphql`
    fragment Attendance on Query
      @argumentDefinitions(
        month: { type: "String" }
        year: { type: "String" }
      ) {
      attendance(month: $month, year: $year) {
        date
        inputId
        inputTime
        inputEventType
        outputId
        outputTime
        outputEventType
        colorId
        holiday
      }
      passagePicker {
        id
        text
      }
    }
  `,
);

[steida]

I suppose it works in Facebook, so maybe it's something on my side. But what? https://github.com/jamiebuilds/create-react-context? Or something else?

[taion]

I'm seeing a bug here was well where the fragment container is receiving updated props, but the underlying component is not.

If I drop a breakpoint in getDerivedStateFromProps in the fragment container constructor, it appears that this never gets called.

Could this be a consequence of the way the containers lazy-construct their components? Is this something that's fixed in the internal cut of React, but not in the public release?

[sibelius]

are you using react 16.3.0? https://github.com/facebook/relay/releases/tag/v1.6.0

[taion]

16.3.2

[jstejada]

Hmm @taion, do you have a consistent repro case I could look at?

[taion]

Let me see if I can put one together easily from the TodoMVC example. I am pretty consistently seeing any breakpoints in getDerivedStateFromProps in fragment container never getting hit against React 16.3.2, though.

[bvaughn]

Hi everyone. Sorry to see this bug. 😦

React 16.4 was release yesterday evening. Would you mind updating and seeing if it resolves the issue? If not, I'll dig deeper.

PS:

when I add key={Math.random{}} wrapped components are properly rerendered.

This forces React to always discard the instance and create a new one (re-create rather than update), so I wouldn't advise it for performance purposes.

[taion]

I'll take a look. Sorry for dragging my feet on the repro; it's not easy to set up a runnable Relay example, and I need a new release of graphql to be published to be able to even set up something that runs in-browser due to some packaging quirks.

[taion]

Still not working. I'm going to put together a conceptual repro that shows the pattern used here, instead of using Relay per se. I think it will be easier to understand.

[taion]

@bvaughn @jstejada

Here's a CodeSandbox showing what I believe to be the issue: https://codesandbox.io/s/yq9l8y494j.

Note that this is on React 16.4.0. The key part is this:

function createLazyComponentWithGDSFP() {
  let ConcreteComponent;

  return (props, context) => {
    if (!ConcreteComponent) {
      ConcreteComponent = class ConcreteComponent extends React.Component {
        state = {
          value: -1
        };

        static getDerivedStateFromProps({ value }) {
          return { value };
        }

        render() {
          return <div>{this.state.value}</div>;
        }
      };
    }

    return new ConcreteComponent(props, context);
  };
}

This essentially demonstrates the pattern used as follows:
https://github.com/facebook/relay/blob/8ef72944e137dc475a519e5ccab6125b5db9f51f/packages/react-relay/modern/buildReactRelayContainer.js#L50-L101

This buildReactRelayContainer is used by createFragmentContainer:
https://github.com/facebook/relay/blob/8ef72944e137dc475a519e5ccab6125b5db9f51f/packages/react-relay/modern/ReactRelayFragmentContainer.js#L298-L309

To wit, createFragmentContainer returns what I believe is called an indeterminate component internally. The component class itself is constructed lazily, and the component returns a component instance when rendered.

It looks like when this happens, gDSFP doesn't get picked up. It in fact never even gets called. Note, however, that cWRP still works as expected.

Note that the key thing here is not the memoization per se; it's that the component class is constructed lazily, and that the component itself is indeterminate and _returns_ an instance, rather than being an instance.

I suspect this must work against master or some v17 alpha of React. This issue means that fragment containers can't receive changed props. It's severe enough that, as far as I'm aware, nobody is using OSS Relay 1.6 for any web deployments.

[taion]

One sec, I was missing the change from https://github.com/facebook/relay/commit/0573d773af1632dd70b03311cfc30c9b5eb4c0e6, so that example isn't valid. I am still seeing getDerivedStateFromProps not getting called, though, so I need to look closer to see what's happening.

[taion]

Okay, my current hypothesis is that this is actually due to https://github.com/facebook/relay/commit/7ef657d66a7fa985c570068b414357b50385cc19, which added an expectation that the context.relay object would be mutated in-place.

A number of us were using various custom query renderers that did not in fact mutate context.relay in-place.

[taion]

Yup, that's exactly it. The commit above added a hard expectation that context.relay would be mutated in-place rather than replaced entirely.

I'm updating Found Relay to match that assumption in https://github.com/relay-tools/found-relay/pull/140. cc @robrichard, who will need to do the same for relay-query-lookup-renderer.

There's not _exactly_ a bug in Relay here, but adding that assumption is pretty nasty.

[bvaughn]

Good catch, @taion. This particular change was made in order to allow Relay to inject a React legacy context object as a prop (so that it could be referenced within the static gDSFP lifecycle). The mutation change was required so that the forwarded prop wasn't disconnected by an update.

I believe at the time, we were under the impression that no external code should be using or relying on Relay's context. Sounds like that was a bad assumption.

[taion]

Yeah, I figured.

To clarify, these components weren't actually using or relying on the context object. They were _providing_ the relay context object.

Per https://github.com/facebook/relay/pull/1760#issuecomment-304502674, we were encouraged to build query renderer replacements outside of this library, and in fact did so. Given the structure of the code, it was necessary for these components to also provide context.relay.

Given that, it might be helpful API-wise to split out the Relay context provider from <QueryRenderer> itself to continue to enable this pattern, while insulating other users from the details around the context API here.

Query renderer interoperability seems like a good thing to have, and this is in fact the first time there's been an issue due to things happening at the component level.

[bvaughn]

That is also new information to me ^ (but useful to know). Thank you for pointing it out.

I believe at some point, Relay plans to replace legacy context usage with the official React.createContext API (and React.forwardRef). Sounds like that change will also need to be coordinated with external libs.

cc @kassens

[taion]

Yup. Hence my suggestion. Relay Classic in fact separates out Relay.ReadyStateRenderer from Relay.Renderer: https://github.com/facebook/relay/blob/v1.6.0/packages/react-relay/classic/container/RelayReadyStateRenderer.js

Analogously, splitting out something like a FetchedQueryRenderer that contains the guts of QueryRenderer but not the parts that interact with ReactRelayQueryFetcher could make things easier going forward. (Likewise with exporting ReactRelayQueryFetcher!)

[aminland]

Is there an ETA on when this will be fixed?

[taion]

There’s technically not an issue in Relay here. It’s Relay integrations that provide Relay context that need to be updated.

[taion]

Okay, there is a bad interaction with RHL that I'm fixing in https://github.com/gaearon/react-hot-loader/pull/1014.

[sciyoshi]

Thanks @taion, release 4.3.1 of react-hot-loader seems to have fixed the issue on my end! :tada:

[taion]

Can we close this?

[sibelius]

@steida can you check with latest react-hot-loader version?

[steida]

OK, this is the simplified workaround:

// @flow
import * as React from 'react';
import PropTypes from 'prop-types';
import type { Environment } from 'react-relay';
import { withRouter } from 'next/router';

// RelayProvider sets Relay context manually for SSR and pending navigations.
// Check App.getInitialProps, note "await fetchQuery". It replaces QueryRenderer.
// https://github.com/robrichard/relay-context-provider
// TODO: Recheck with every new Relay release.

type RelayProviderProps = {|
  environment: Environment,
  children: React.Node,
  router: {|
    query: Object,
  |},
|};

class RelayProvider extends React.PureComponent<RelayProviderProps> {
  static childContextTypes = {
    // eslint-disable-next-line react/forbid-prop-types
    relay: PropTypes.object.isRequired,
  };

  constructor(props, context) {
    super(props, context);
    this.relayContext = {
      environment: this.props.environment,
      variables: this.props.router.query,
    };
  }

  relayContext: {|
    environment: Environment,
    variables: Object,
  |};

  getChildContext() {
    return {
      relay: this.relayContext,
    };
  }

  componentWillReceiveProps(nextProps) {
    // https://github.com/facebook/relay/issues/2429#issuecomment-391808755
    Object.assign(this.relayContext, {
      environment: nextProps.environment,
      variables: nextProps.router.query,
    });
  }

  render() {
    return this.props.children;
  }
}

export default withRouter(RelayProvider);

Works well in https://github.com/este/este

[voxmatt]

I'm still seeing this behavior without hot-loading and without any router. I just have a renderer that's passing props in the render callback and the child component isn't seeing the new props.

I have not yet upgraded to react 16.3 (I'm on 16.2). Is that a requirement here?

[voxmatt]

Confirmed that moving to React 16.4 solved my problem

[tomconroy]

We are still seeing this issue with React 16.4.1 and found-relay@alpha13, but only in "development" mode (we don't use react-hot-loader, only webpack-hot-middleware). It also only seems to affect compat fragment containers.

[jstejada]

EDIT:e6304cf447b58f9bbf60f8d6f5d95e8bbca090f4 is a fix for a different issue

[jstejada]

This is the same as #2436

[tony]

Anyone still getting this issue with fragment containers not rendering with react-hot-loader? react 16.8.6, react-relay 2.0.0, hot-loader 4.12.8

Sorry for bringing this up here - but either the original issue actually wasn't remedied (maybe, I could be wrong) or we may need to organize to define the issue better.

At this point, I don't know if this is just me, a relay issue, or a hot-loader issue. Just want to keep the communication line open.