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

Framework: Use standalone shallowequal instead of react/lib's #1463

Merged
merged 1 commit into from
Dec 10, 2015

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Dec 10, 2015

In preparation for React 0.14, cf #776 #786 #787 and friends.

To test:
Make sure the Reader still works.

CC @blowery @aduth

@ockham ockham added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Dec 10, 2015
@ockham ockham self-assigned this Dec 10, 2015
@aduth
Copy link
Contributor

aduth commented Dec 10, 2015

Tests well for me 👍 (Specifically, changing lists in the Reader)

Two points to make:

  • All new dependencies should be added to CREDITS.md, per contribution guidelines
  • On a few occasions, I've been tempted to bring in Dan Abramov's react-pure-render helpers, which include a shallowEqual utility. We could eliminate the extra dependency if we were to use only this module instead. That said, I've not yet had a need for the helpers, so maybe this is a bit too preemptive 😄

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 10, 2015
@ockham ockham force-pushed the update/standalone-shallow-equal branch from 2da50e4 to 918dfb5 Compare December 10, 2015 14:55
@ockham
Copy link
Contributor Author

ockham commented Dec 10, 2015

Two points to make:

Uh right, I keep forgetting. Added & squashed.

  • On a few occasions, I've been tempted to bring in Dan Abramov's react-pure-render helpers, which include a shallowEqual utility. We could eliminate the extra dependency if we were to use only this module instead. That said, I've not yet had a need for the helpers, so maybe this is a bit too preemptive 😄

Yeah, shallowequal docs says it was actually taken from that library (instead of directly from React). I was considering using react-pure-render's, too, but came to the same YAGNI conclusion (even if we actually might need it at some point, I don't think there's much sense in adding it already). We just need to remember to remove the shallowequal dependency whenever we add react-pure-render.

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 10, 2015
@aduth
Copy link
Contributor

aduth commented Dec 10, 2015

In that case, I think this is good to go 👍

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 10, 2015
ockham added a commit that referenced this pull request Dec 10, 2015
Framework: Use standalone shallowequal instead of react/lib's
@ockham ockham merged commit fed1679 into master Dec 10, 2015
@ockham ockham deleted the update/standalone-shallow-equal branch December 10, 2015 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants