Props selectedStartDate and selectedEndDate does not work

[YevhenLobanov]

In index file those props are set to null by default and not handled when props are changed outside

[peacechen]

Thanks for catching that. Would you mind creating a PR?

[YevhenLobanov]

Hello, I guess i need permission for this

On Thu, May 17, 2018 at 6:02 PM, Peace notifications@github.com wrote:

Thanks for catching that. Would you mind creating a PR?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stephy/CalendarPicker/issues/122#issuecomment-389898609, or mute the thread https://github.com/notifications/unsubscribe-auth/AfX2VpVIDu4zv6rUqy3GoEOyaecX3BGnks5tzZEUgaJpZM4UDMsc .

[peacechen]

Standard procedure for submitting a PR is to fork the repo, commit your changes to your fork, then select the button "New pull request" in your fork repo.

[prashantm8]

also not working for me. please help me.

[YevhenLobanov]

@prashantm8 take a look in PR #123

[peacechen]

5.19.0 has been published. Please try it out and close this issue if it works for you.

[prashantm8]

it worked for me. thanks

[YevhenLobanov]

@peacechen Could You please update npm package also, it is still 5.18

[peacechen]

It has already been published. There seems to be a delay with npm's system. The confirmation email I received was:

A new version of the package react-native-calendar-picker (5.19.0) was published at 2018-05-22T14:07:19.431Z from w.x.y.z. The shasum of this package was 3d1e5c7a43da5d55a71b0795e4ec648150512b67.

[YevhenLobanov]

Works fine after merge

[rolfb]

This seems to be causing a bug when the component updates as it drops existing state and rewrites from props.

[rolfb]

To clarify, if the calendar has internal state with new values for selectedStartDate and selectedEndDate and the componentDidUpdate triggers for a different reason than updating these two values from the outside, these values will get reset even if it has a newer internal value.

https://github.com/stephy/CalendarPicker/blob/master/CalendarPicker/index.js#L69-L78

[peacechen]

Good investigative work. Would you try changing the comparison with this.props.selectedStartDate to this.state.selectedStartDate ?

https://github.com/stephy/CalendarPicker/blob/master/CalendarPicker/index.js#L71

... and the equivalent for selectedEndDate.

It should be comparing its current & previous states instead of against props.

[rolfb]

I could, but I don't know the full intent of the code in the componentDidUpdate method - it looks like it should be refactored into something more comprehensible. componentDidUpdate should not call setState as that can create an infinite loop. The boolean value controlling when to call setState is a code smell as well. Perhaps @YevhenLobanov can show us his usage of this plugin so I can better understand the initial problem?

[peacechen]

Setting state in componentDidUpdate is standard practice in React. Otherwise there'd be no way to update state when props change. TBH this pattern is a fundamental issue with React. I'm open to suggestions on how to improve the implementation. We may need to leverage shouldComponentUpdate, one of React's ways of addressing and optimizing prop changes.

[rolfb]

Odd, didn't know it was "standard practice" - I've never had to resort to using it that way. But as mentioned before, I can refactor this when someone explains componentDidUpdate's requirements.

[peacechen]

I would like to see your approach to updating state when props change. If there's a better way, I'm all for improving that. The doStateUpdate flag is a way to gather all the prop changes that affect state, and call setState once at the end. Otherwise there would be multiple setState calls in componentDidUpdate which is a performance hit.

[rolfb]

I'll have a look later, but still unclear about the acceptance criteria. Fwiw, multiple calls to setState when not passing a function ala setState((state) => // do something) is batched into a single operation by React and not a performance hit as far as I know -- but the order of batching is not deterministic and subject to race conditions if you change the same key multiple times.

[peacechen]

I've heard that setState is supposed to gather up multiple calls, but in practice I've found that claim to be inaccurate. In many instances I've seen back-to-back setState calls result in multiple re-renders. If there's a guarantee that setState is optimized for that, we can refactor the code, but I don't trust it to behave optimally.

[rolfb]

I see your point, haven't checked to see how long it batches for or what triggers it to execute it all.