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

Fix bug when passing a new `renderMonthTitle` on each render #1718

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cedrics
Copy link

commented Jul 2, 2019

react-dates version
e.g. react-dates@20.2.5

Describe the bug
When passing in a different function for renderMonthTitle on each render call, the DayPicker is hidden on subsequent rerenders.
Source code (including props configuration)

If you change the DateRangePickerWrapper to the following code, you can reproduce the issue in storybook.

<DateRangePicker
  startDate={this.state.startDate}
  startDateId="your_unique_start_date_id"
  endDate={this.state.endDate}
  endDateId="your_unique_end_date_id"
  onDatesChange={({ startDate, endDate }) => this.setState({ startDate, endDate })}
  focusedInput={this.state.focusedInput}
  onFocusChange={focusedInput => this.setState({ focusedInput })}
  renderMonthText={() => "test"}
/>

Select a start date and then select an end date that is before the start date (which forces a rerender and not the daypicker to destroyed and readded).

Screenshots/Gifs

bug-react-dates

Bug Description

When the renderMonthTitle functions changes the calculated monthTitleHeight gets reset (See code). However, the CalendarMonth currently only computes the height on mount. This changes the behavior of the component to recompute the height whenever it is passed the callback to set it.

@ljharb
Copy link
Collaborator

left a comment

Thanks, could you include a regression test?

@coveralls

This comment has been minimized.

Copy link

commented Jul 2, 2019

Coverage Status

Coverage decreased (-0.1%) to 84.781% when pulling dbdfc2f on cedrics:fix-rerender-issue-when-focus-does-not-change into 121fc44 on airbnb:master.

@cedrics

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

@ljharb sure, I have added some regression tests. As the code to test uses both refs and setTimeout the test is fairly complicated and kind of ugly 😄 If you have some pointers how to improve it, I would appreciate it.

I also stole the describeIfWindow function from the DateRangePicker spec. Should I extract it to a common util method? If so should I just add it to test/_helpers

@@ -46,5 +48,50 @@ describe('CalendarMonth', () => {
expect(typeof isVisible).to.equal('boolean');
expect(wrapper.find('#month-element').exists()).to.equal(true);
});

describeIfWindow('setMonthTitleHeight', () => {
const awaitChange = fn => new Promise((resolve, reject) => {

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 3, 2019

Collaborator

This seems very brittle; it’s explicitly racing against the change. These tests should find a way to explicitly wait for the proper time instead.

/>,
);

return awaitChange(() => expect(setMonthTitleHeightStub.callCount).to.equal(1));

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 3, 2019

Collaborator
Suggested change
return awaitChange(() => expect(setMonthTitleHeightStub.callCount).to.equal(1));
return awaitChange(() => expect(setMonthTitleHeightStub).to.have.property('callCount', 1));

this has a nicer failure message; same throughout

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

Yes, moving it to a common place would be great.

return;
}

setTimeout(checkAndWait, 10);

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 3, 2019

Collaborator

Sorry if i wasn’t clear - any usage of setTimeout at all, or anything based on the concept of waiting for some amount of time, is inherently brittle and shouldn’t be used. We need access to the actual async behavior so we can explicitly await on it.

This comment has been minimized.

Copy link
@cedrics

cedrics Jul 3, 2019

Author

I am not sure what you are proposing. Could you describe in more concrete terms how this change might look like?

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 3, 2019

Collaborator

Since you want to wait until wrapper.instance().setMonthTitleHeightTimeout has ran, at the very least, you'd want to poll until that had become null. However, in this case, I think you could do something like:

let timeout = wrapper.instance().setMonthTitleHeightTimeout;
const stub = sinon.stub(wrapper.instance(), 'setMonthTitleHeightTimeout');
stub.get(() => timeout);
const nulled = new Promise((resolve) => {
  stub.set((value) => {
    if (timeout !== null && value === null) resolve();
    timeout = value;
  });
});
return nulled.then(() => {
  expect(whatever);
});

This comment has been minimized.

Copy link
@cedrics

cedrics Jul 4, 2019

Author

Thanks, I think I get what you want now. Your solution is already coupled to the actual implementation, so a much more straight forward solution would be to just stub out setTimeout for the this test and make it synchronous. The asynchronicity is irrelevant for this test. This would reduce coupling to between the implementation and the tests but may result in some weird behavior if some other piece of code relies on setTimeout be asynchronous. Your call 😸

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 5, 2019

Collaborator

Coupling tests to the implementation is fine as long as changing the impl in a way that breaks it, breaks the tests.

That includes stubbing setTimeout, which could certainly work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.