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

CPLAT-935 Add PureComponentMixin #499

Merged
merged 19 commits into from
Jul 16, 2020
Merged

CPLAT-935 Add PureComponentMixin #499

merged 19 commits into from
Jul 16, 2020

Conversation

aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Jun 9, 2020

Motivation

Consumers sometimes ask how to use React.PureComponent with OverReact.

Changes

  1. Add PureComponentMixin which will cause a component to behave like React.PureComponent.
  2. Add tests.

Release Notes

Add PureComponentMixin to allow consumers to create components that mimic React.PureComponent behavior.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review: @greglittlefield-wf @kealjones-wk

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Client Platform member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

@aviary-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@aaronlademann-wf aaronlademann-wf marked this pull request as ready for review June 9, 2020 21:45
+ And a custom Equality that is safe for ReactElement / Function() as props / state values in dart2js
@aaronlademann-wf aaronlademann-wf changed the title Add PureUiComponent mixin CPLAT-935 Add PureUiComponent mixin Jun 30, 2020
lib/src/component/pure_ui_component.dart Outdated Show resolved Hide resolved
lib/src/component/pure_ui_component.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple more things, really close!

lib/src/component/pure_ui_component.dart Outdated Show resolved Hide resolved
lib/src/component/pure_ui_component.dart Outdated Show resolved Hide resolved
lib/src/component/pure_ui_component.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+10

+ To avoid conflicts with many pre-existing implementations in consumer libs
+ And export from over_react.dart instead of component.dart
@aaronlademann-wf aaronlademann-wf changed the title CPLAT-935 Add PureUiComponent mixin CPLAT-935 Add PureComponentMixin Jul 16, 2020
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+10

+ Becaues otherwise, `propTypes` cannot be overridden without analysis issues for components mixing in PureComponentMixin
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+10

/// Main entry point for `PureComponentMixin` testing
main() {
group('PureComponentMixin', () {
const initialChildren = ['initial'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're only testing prop updates here. Should we also be testing the effect of state updates/non-updates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a huge deal since the implementation is the same as for props 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test at the very beginning that uses forceUpdate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see it now 👍

@greglittlefield-wf
Copy link
Contributor

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from RM

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

Successfully merging this pull request may close these issues.

7 participants