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

Stateful component mixin #38

Conversation

greglittlefield-wf
Copy link
Contributor

Depends on #28

Ultimate problem:

Previously, it was impossible to create a new stateful component that extended from a non-stateful component.

This has been worked around by ensuring that the base component was stateful as well, but that isn't always possible (e.g., the component comes from another library).

How it was fixed:

  • Split stateful component behavior into mixin
    • Bonus: UiStatefulComponent now implements UiComponent!
  • Simplify declaration of FluxUiComponent/FluxUiStatefulComponent by using new mixin
    • Bonus: FluxUiStatefulComponent now implements both FluxUiComponent and UiStatefulComponent!

Testing suggestions:

  • Verify all tests pass (especially in dart2js).
  • As an integration test, Pull into WSD and verify there are no analyzer errors, and no runtime errors in Dartium or dar2tjs.

Potential areas of regression:

Advanced component inheritance


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

@aviary2-wf
Copy link

Raven

Number of Findings: 0

@codecov-io
Copy link

Current coverage is 97.43% (diff: 100%)

Merging #38 into master will decrease coverage by 0.01%

@@             master        #38   diff @@
==========================================
  Files            27         27          
  Lines          1286       1283     -3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1253       1250     -3   
  Misses           33         33          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update b533cd2...063a1a9


/// A mixin that adds support for strongly-typed state to a [UiComponent].
abstract class UiStatefulMixin<TState extends UiState>
// Implement react.Component instead of UiComponent so we don't run into https://github.com/dart-lang/sdk/issues/14729
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner to notate this in the doc comment 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.

I don't think so, unless you think this information is beneficial to the audience of the doc comment.

@@ -91,7 +91,7 @@ typedef TProps BuilderOnlyUiFactory<TProps extends UiProps>();
///
/// Extends [react.Component].
///
/// Related: [UiStatefulComponent]
/// For strongly-typed state, mix in [UiStatefulMixin] or extend from [UiStatefulComponent].
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be fair to say that using UiStatefulMixin is considered best practice, instead of extending UiStatefulComponent? If so should we just deprecate UiStatefulComponent?

@greglittlefield-wf
Copy link
Contributor Author

Closing this until issues discussed offline (in dart2js compilation, I believe) around FluxUiComponent are fixed.

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.

5 participants