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

Remove dependency on React internals #151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tschaub
Copy link

@tschaub tschaub commented Aug 5, 2016

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.

@tschaub tschaub force-pushed the pure-render-mixin branch from eb08956 to 1824dba Compare August 5, 2016 02:38
@AlanFoster
Copy link
Member

I like the suggestion @tschaub 👍

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

@tschaub tschaub force-pushed the pure-render-mixin branch from 1824dba to 0a60670 Compare August 11, 2016 03:56
@tschaub
Copy link
Author

tschaub commented Aug 11, 2016

@AlanFoster - rebased. Thanks for checking it out.

@tschaub
Copy link
Author

tschaub commented Aug 15, 2016

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

@tschaub
Copy link
Author

tschaub commented Sep 23, 2016

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
Copy link
Author

tschaub commented Oct 18, 2016

It looks like this will even be more important for people upgrading React (when internal lib modules are moved or changed). See facebook/react#7770 (comment).

@gaearon
Copy link

gaearon commented Oct 19, 2016

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
Copy link
Member

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

@tschaub
Copy link
Author

tschaub commented Nov 17, 2016

@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).

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

Successfully merging this pull request may close these issues.

3 participants