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-3119: Move BuiltReduxUiComponent and BuiltReduxUiProps out of experimental #144

Conversation

jacehensley-wf
Copy link
Contributor

Ultimate problem:

BuiltReduxUiComponent and BuiltReduxUiProps are marked experimental but are used in production, they should be move out of experimental.

How it was fixed:

  • Move BuiltReduxUiComponent and BuiltReduxUiProps out of experimental.
  • Continue to export them from the experimental.dart entry point to avoid breakages.
  • Add a note about the two classes getting deprecated in a future release in favor of a new way of building built_redux components.

Testing suggestions:

  • Verify tests pass

Potential areas of regression:

  • Exporting of BuiltReduxUiComponent and BuiltReduxUiProps

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

@jacehensley-wf jacehensley-wf requested a review from a team as a code owner March 8, 2018 17:47
@aviary2-wf
Copy link

Module Findings

No security relevant content detected. Please review for security relevance and request security review as needed.

@codecov-io
Copy link

Codecov Report

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

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   94.51%   94.51%           
=======================================
  Files          33       33           
  Lines        1601     1601           
=======================================
  Hits         1513     1513           
  Misses         88       88

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.

I don't feel comfortable exposing this as a non-deprecated stable API.

Going to discuss this with the team early Monday.

@travissanderson-wf
Copy link

@greglittlefield-wf we had a very lengthy conversation about this in the "Support: Datatables & Redux Toolbars" hipchat room as well as a quick google hangout. The hard line is that we can't consume this in a production app as production code if it is marked as experimental/deprecated/whatever other annotation you want to use to exclude it from semver. Please take your concerns to that chat, if you could read thru the existing conversation first that would be great.

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.

  • No analyzer errors when pulled into a repo consuming both package:over_react/experimental.dart package:over_react/over_react.dart
  • All tests are still run in CI
  • CI passes

+10

@greglittlefield-wf greglittlefield-wf merged commit 3dedefa into Workiva:master Mar 15, 2018
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.

8 participants