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-5909 Update error boundary component to use the init method #300

Merged
merged 10 commits into from
May 30, 2019

Conversation

joebingham-wk
Copy link
Contributor

@joebingham-wk joebingham-wk commented May 22, 2019

Motivation

The init lifecycle method was introduced in part to allow for set to be state prior to component mounting. With the current code generation script, the typing conflicts because the generated code expects a JsBackedMap while react-dart accepts any Map. This results in a runtime error.

Changes

  • Modify the generation script to cast the passed in Map to a JsBackedMap.
  • Update the stateful component test to use the init method to ensure it behaves the same as getInitialState.
  • Replace getInitialState with init within error_boundary_mixins.dart.

Release Notes

Review

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

Please review:
@aaronlademann-wf @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
    • Steps from PR author:
      • Pull this branch down
      • Run webdev serve web
      • Verify the error boundary component works while using the init() lifecycle method.
    • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

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

Cast Map being passed into the state setter to a JsBackedMap
@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.

@joebingham-wk joebingham-wk changed the title CPLAT-5816 Update the state setter to allow for maps to be passed in CPLAT-5909 Update error boundary component to use the init method May 23, 2019
@joebingham-wk joebingham-wk added the Hold Hold merges label May 23, 2019
@aaronlademann-wf aaronlademann-wf added this to the 3.1.0 milestone May 29, 2019
@@ -247,6 +247,9 @@ class ImplGenerator {
..writeln()
..writeln(' @override')
..writeln(' set state(Map value) {')
..writeln(' assert(value is JsBackedMap, ')
..writeln(' \'Component2.state may only be set to a JsBackedMap, \'')
..writeln(' \'and must not be set outside of the react-dart internals.\');')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this message for helpful to the consumer by instructing them to use the initializeState method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good call. Because it's the general state setter I updated the message to also mention setState. Let me know if the new message could be improved as well!

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

@joebingham-wk this is looking good, but the build is failing.

This is because your changes are dependent on Workiva/react-dart#187, but the dependency override has not been updated to reflect that in pubspec.yaml.

You were correct to point out the dependency in your PR description, and to place the Hold label. However, when working on PRs / branches that depend on one another, you should also go ahead and add/update the dependency override within pubspec.yaml so that we can verify that the build will pass using the changes that are external to this lib.

dependency_overrides:
  react:
    git:
      url: https://github.com/cleandart/react-dart.git
-      ref: 5.1.0-wip
+      ref: CPLAT-5908-Add-initializeState

Then, once the CPLAT-5908-Add-initializeState branch merges into react-dart 5.1.0-wip, you can switch it back to 5.1.0-wip. If the PR was not going into an integration branch like this one is, you would just remove the dependency override altogether at that time.

@joebingham-wk
Copy link
Contributor Author

Awesome, that explanation helps a lot. Thank you!

Copy link
Contributor

@aaronlademann-wf aaronlademann-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

@aaronlademann-wf
Copy link
Contributor

+10

@Workiva/release-management-pp

@rmconsole6-wk rmconsole6-wk deleted the CPLAT-5816-update-state-setter branch May 30, 2019 20:47
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.

6 participants