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-6287: Promote missing over_react part to severe log (fail the build) #314

Merged
merged 2 commits into from
Jul 10, 2019

Conversation

evanweible-wf
Copy link
Contributor

@evanweible-wf evanweible-wf commented Jul 2, 2019

CPLAT-6287

Issue Status

This is a breaking change and has been PR'd to the 3.0.0 wip branch.

Motivation

The generated .over_react.g.dart part file is required for components to function on Dart2. Currently, the builder only logs a warning when the part file is missing, which could lead to a scenario where a consumer omits the part, ignores or doesn't see the warning, and runs into a runtime exception. It would be better to promote this error to one that fails the build so that it can be fixed immediately.

Changes

This PR promotes the log for the missing over_react part to the SEVERE level, which in turn causes the build to fail. In other words, this failure is being promoted from a runtime error to a build-time error.

Changes

Release Notes

Review

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

Please review:

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code

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

@aviary3-wk
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.

evanweible-wf added a commit that referenced this pull request Jul 2, 2019
This will cause the build to fail, which is a breaking change as
previously the builder would only warn. This change is being made
because the part directive is required to function correctly on
Dart 2 (without it, the component will fail at runtime). In other
words, we are promoting the failure from runtime to buildtime.
@evanweible-wf evanweible-wf force-pushed the promote_missing_part_to_severe branch from 412bdcd to c330c63 Compare July 2, 2019 23:16
@rmconsole3-wf rmconsole3-wf changed the title Promote missing over_react part to severe log (fail the build) CPLAT-6287 Promote missing over_react part to severe log (fail the build) Jul 2, 2019
@charliekump-wf charliekump-wf changed the title CPLAT-6287 Promote missing over_react part to severe log (fail the build) CPLAT-6287: Promote missing over_react part to severe log (fail the build) Jul 2, 2019
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

@evanweible-wf
Copy link
Contributor Author

QA +1

  • Build emits a severe log now instead of a warning when part is missing

@Workiva/release-management-p

@rmconsole5-wk rmconsole5-wk merged commit c8b8d3b into 3.0.0-wip+dart2 Jul 10, 2019
@rmconsole5-wk rmconsole5-wk deleted the promote_missing_part_to_severe branch July 10, 2019 16:42
kealjones-wk added a commit that referenced this pull request Aug 6, 2019
* 3.0.0-wip:
  Address CR feedback
  Point to published version of react
  Add support for SyntheticAnimationEvent / SyntheticTransitionEvent
  Backport type loosening of DomProps.componentFactory in 3.1.0-wip, since it could break some usages
  Avoid shadowing `type` argument
  Improve error message for props/state variables with initializers
  Changelog entry for #314.
  Promote missing over_react part to a SEVERE log.
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.

None yet

7 participants