Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bailing out doesn't work properly in lazy components with default props #17151

Closed
jddxf opened this issue Oct 20, 2019 · 7 comments · Fixed by #18539
Closed

Bailing out doesn't work properly in lazy components with default props #17151

jddxf opened this issue Oct 20, 2019 · 7 comments · Fixed by #18539

Comments

@jddxf
Copy link
Contributor

jddxf commented Oct 20, 2019

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Bailing out doesn't work properly in lazy components with default props. It seems we're incorrectly comparing unresolved props (oldProps) with resolved props (newProps).

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
https://codesandbox.io/s/stoic-curran-3otbb

What is the expected behavior?
In the example above, componentDidUpdate shouldn't have been called when the button is clicked.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

@stale
Copy link

stale bot commented Jan 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 19, 2020
@stale
Copy link

stale bot commented Jan 26, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jan 26, 2020
@gaearon gaearon reopened this Apr 1, 2020
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 1, 2020
@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

Can you turn this into a failing test please?

@jddxf
Copy link
Contributor Author

jddxf commented Apr 4, 2020

@gaearon Sure.

it('resolves defaultProps without breaking bailout due to unchanged props and state', async () => {
  class LazyImpl extends React.Component {
    static defaultProps = {value: 0};

    render() {
      const text = `${this.props.label}: ${this.props.value}`;
      return <Text text={text} />;
    }
  }

  const Lazy = lazy(() => fakeImport(LazyImpl));

  const instance1 = React.createRef(null);
  const instance2 = React.createRef(null);

  const root = ReactTestRenderer.create(
    <>
      <LazyImpl ref={instance1} label="Not lazy" />
      <Suspense fallback={<Text text="Loading..." />}>
        <Lazy ref={instance2} label="Lazy" />
      </Suspense>
    </>,
    {
      unstable_isConcurrent: true,
    },
  );
  expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
  expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  await Promise.resolve();

  expect(Scheduler).toFlushAndYield(['Lazy: 0']);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  // Should bailout due to unchanged props and state
  instance1.current.setState(null);
  expect(Scheduler).toFlushAndYield([]);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  // Should bailout due to unchanged props and state
  instance2.current.setState(null);
  expect(Scheduler).toFlushAndYield([]);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
});

it('resolves defaultProps without breaking bailout in PureComponent', async () => {
  class LazyImpl extends React.PureComponent {
    static defaultProps = {value: 0};
    state = {};

    render() {
      const text = `${this.props.label}: ${this.props.value}`;
      return <Text text={text} />;
    }
  }

  const Lazy = lazy(() => fakeImport(LazyImpl));

  const instance1 = React.createRef(null);
  const instance2 = React.createRef(null);

  const root = ReactTestRenderer.create(
    <>
      <LazyImpl ref={instance1} label="Not lazy" />
      <Suspense fallback={<Text text="Loading..." />}>
        <Lazy ref={instance2} label="Lazy" />
      </Suspense>
    </>,
    {
      unstable_isConcurrent: true,
    },
  );
  expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
  expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  await Promise.resolve();

  expect(Scheduler).toFlushAndYield(['Lazy: 0']);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  // Should bailout due to shallow equal props and state
  instance1.current.setState({});
  expect(Scheduler).toFlushAndYield([]);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

  // Should bailout due to shallow equal props and state
  instance2.current.setState({});
  expect(Scheduler).toFlushAndYield([]);
  expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
});

@gaearon
Copy link
Collaborator

gaearon commented Apr 4, 2020

Send a PR?

@jddxf
Copy link
Contributor Author

jddxf commented Apr 4, 2020

OK

@gaearon
Copy link
Collaborator

gaearon commented Apr 4, 2020

Feel free to add a fix if you know where the bug is 😳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants