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

UIP-2258 Workaround for Dart 1.23 strong mode issue with MapViewMixin #65

Merged
merged 2 commits into from
May 2, 2017

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented May 1, 2017

Ultimate problem:

A strong mode change was made in Dart 1.23 that disallows conflicting private members in mixins: dart-lang/sdk#28809.

This was causing issues in w_filing when manually mixing in MapViewMixin/PropsMapViewMixin https://github.com/Workiva/w_filing/blob/1c9a406b59daf3c178d0c69336690c5ce28763f0/lib/src/shared/services/configs/service_props_base.dart#L3

This would have been an issue in over_react if UiProps was in a separate library from these classes.

How it was fixed:

Declare conflicting _props getter in common base class.

Testing suggestions:

  • Verify unit tests pass
  • Verify analyzer passes in Dart 1.22.0
  • Pull the branch into w_filing in Dart 1.23.0 and verify that tehre are no strong mode errors in ServicePropsBase.

Potential areas of regression:

UiProps class


FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

@aviary2-wf
Copy link

Raven

Number of Findings: 0

Copy link

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

Good catch and comments explaining.

@gavinmulligan-wf
Copy link

+1

@johnbland-wf
Copy link

Good catch on the 3bfd938. +1 refresh

@gavinmulligan-wf
Copy link

+1

@rmconsole3-wf rmconsole3-wf changed the title Workaround for Dart 1.23 strong mode issue with MapViewMixin UIP-2258 Workaround for Dart 1.23 strong mode issue with MapViewMixin May 1, 2017
@codecov-io
Copy link

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   97.69%   97.69%           
=======================================
  Files          28       28           
  Lines        1381     1381           
=======================================
  Hits         1349     1349           
  Misses         32       32

@gavinmulligan-wf
Copy link

+10 - Dev QAed on w_filing and verified the analyzer errors are gone. Nice work @greglittlefield-wf

@leviwith-wf
Copy link
Contributor

QA +1

  • Testing instruction
  • Dev +1's
  • Dev/QA +10
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

@leviwith-wf leviwith-wf merged commit b9c2517 into Workiva:master May 2, 2017
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