search

onefinestay / react-daterange-picker

Other
563 stars 208 forks source link

Remove dependency on React internals #151

Open tschaub opened 7 years ago

tschaub commented 7 years ago

I'm hoping you'd be willing to move away from the dependency on react-addons-pure-render-mixin. Since this module requires functionality from React internals (react/lib/ReactComponentWithPureRenderMixin), it makes it so an application cannot use react-daterange-picker and load React from a CDN.

Since there is already a PureRenderMixin in this package, one alternative would be to use that. Or, since this does additional checks for moment, things could be split into one mixin for shallow comparisons with moment and one mixin for shallow comparisons without moment.

I'll be happy to make changes to this if you're interested in moving away from react-addons-pure-render-mixin.

AlanFoster commented 7 years ago

I like the suggestion @tschaub :+1:

Would you mind rebasing this and I can take a closer look at this too.

tschaub commented 7 years ago

@AlanFoster - rebased. Thanks for checking it out.

tschaub commented 7 years ago

@AlanFoster - let me know if you think this should be handled another way.

tschaub commented 7 years ago

Is there a way I can help get this in? Curious if anybody else is also interested in using React from a CDN (or separate bundle) and this date picker together.

tschaub commented 7 years ago

It looks like this will even be more important for people upgrading React (when internal lib modules are moved or changed). See https://github.com/facebook/react/issues/7770#issuecomment-253899837.

gaearon commented 7 years ago

To be fair, addons imported via packages will keep on working. If you don't use /lib/ explicitly in your code it's safe.

Still, I would suggest using React.PureComponent base class (new feature in React 15.3.0 and higher).

AlanFoster commented 7 years ago

@tschaub I've been merging a lot of PRs for a 2.x release; Mind re-basing this, and we can get this shipped? :+1:

tschaub commented 7 years ago

@AlanFoster rebased (after a long delay).

This uses the existing util/PureRenderMixin in this repo. I didn't take time to make things work with React.PureComponent as suggested above, since I'm not sure if you want to depend on 15.3 (though that sounds like a good way to go).