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

Context support #135

Merged
merged 32 commits into from
Dec 14, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
d2e4beb
Port over initial stab at context
greglittlefield-wf Oct 26, 2017
3ec9436
Fix breaking change to initComponentInternal
greglittlefield-wf Oct 26, 2017
e1a44b0
Use internal pattern for individual context keys
greglittlefield-wf Oct 27, 2017
27fea39
Fix JS, conditionally declare context pieces on config
greglittlefield-wf Oct 27, 2017
2408a09
Generate JS files
greglittlefield-wf Oct 27, 2017
f772419
Cleanup, naming, docs
greglittlefield-wf Oct 27, 2017
bb04f63
Ignore prevContext since it isn't supported in React 16
greglittlefield-wf Oct 27, 2017
b80ac0a
Generate JS
greglittlefield-wf Oct 27, 2017
bb0bc6c
Rename to handleGetChildContext
greglittlefield-wf Oct 27, 2017
9989c2e
Add prev/nextContext and update docs
johnbland-wf Oct 27, 2017
2e7d74b
Added logging to js build
johnbland-wf Oct 27, 2017
f605e0d
Added lifecycle call checks to LifecycleTest
johnbland-wf Oct 27, 2017
88bb52f
Fix errors in react_test examples
johnbland-wf Oct 27, 2017
b410e62
Fix errors in ref_test examples
johnbland-wf Oct 27, 2017
a8585e8
Fix errors in speed_test examples
johnbland-wf Oct 27, 2017
2fba623
Fix errors in get_dom_node_test examples
johnbland-wf Oct 27, 2017
cd9ddaf
Adjust context component to test childContextKeys
johnbland-wf Oct 27, 2017
846c0f1
Update dart_helpers comment
johnbland-wf Nov 2, 2017
987b995
Merge branch 'master' of github.com:cleandart/react-dart into context
johnbland-wf Nov 30, 2017
6e31305
Clean-up / Removed componentDidUpdateWithContext
johnbland-wf Nov 30, 2017
61c6609
Added testing steps to README
johnbland-wf Nov 30, 2017
7861c20
Update example to showcase state updates
johnbland-wf Dec 1, 2017
804ac5e
Use context directly vs transferring context
johnbland-wf Dec 1, 2017
64fbaae
Add JS build to README
johnbland-wf Dec 1, 2017
5f4d403
recieves -> receives
johnbland-wf Dec 1, 2017
dc0e5af
Updated example component to show context updates to grandchildren
johnbland-wf Dec 1, 2017
92fbc53
Fix lifecycle calls and add test coverage
johnbland-wf Dec 1, 2017
723caa5
Remove forced context update
johnbland-wf Dec 5, 2017
f791d0d
Documentation updates
johnbland-wf Dec 5, 2017
f0d8b3a
Use nextContext vs unjsify’ing it
johnbland-wf Dec 5, 2017
282914b
Test updates
johnbland-wf Dec 5, 2017
3602963
Remove context test methods in JS
johnbland-wf Dec 5, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add prev/nextContext and update docs
  • Loading branch information
johnbland-wf committed Oct 27, 2017
commit 9989c2ea827232c056f0e89134ea3983acd583f5
107 changes: 103 additions & 4 deletions lib/react.dart
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,12 @@ abstract class Component {
_initProps(props);
}

/// Initializes context
_initContext(context) {
_context = new Map.from(context ?? {});
this.context = new Map.from(context ?? {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

#nit To reduce garbage, this could be const {}.


/// Call `transferComponentContext` to get state also to `_prevContext`
Copy link
Collaborator

Choose a reason for hiding this comment

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

#nit This should not be a doc comment

transferComponentContext();
}

_initProps(props) {
Expand All @@ -108,15 +112,30 @@ abstract class Component {

initStateInternal() {
this.state = new Map.from(getInitialState());
// Call `transferComponentState` to get state also to `_prevState`

/// Call `transferComponentState` to get state also to `_prevState`
Copy link
Collaborator

Choose a reason for hiding this comment

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

#nit This should not be a doc comment

transferComponentState();
}

/// Private reference to the value of [context] for the upcoming render cycle.
///
/// Useful for ReactJS lifecycle methods [shouldComponentUpdateWithContext], [componentWillUpdateWithContext] and
/// [componentDidUpdateWithContext].
Map _nextContext = null;

/// Private reference to the value of [state] for the upcoming render cycle.
///
/// Useful for ReactJS lifecycle methods [shouldComponentUpdate], [componentWillUpdate] and [componentDidUpdate].
Map _nextState = null;

/// Reference to the value of [context] from the previous render cycle, used internally for proxying
/// the ReactJS lifecycle method and [componentDidUpdateWithContext].
///
/// Not available after [componentDidUpdateWithContext] is called.
///
/// __DO NOT set__ from anywhere outside react-dart lifecycle internals.
Map prevContext;

/// Reference to the value of [state] from the previous render cycle, used internally for proxying
/// the ReactJS lifecycle method and [componentDidUpdate].
///
Expand All @@ -125,18 +144,33 @@ abstract class Component {
/// __DO NOT set__ from anywhere outside react-dart lifecycle internals.
Map prevState;

/// Public getter for [_nextState].
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/_nextState/_nextContext/

///
/// If `null`, then [_nextContext] is equal to [context] - which is the value that will be returned.
Map get nextContext => _nextContext == null ? context : _nextContext;

/// Public getter for [_nextState].
///
/// If `null`, then [_nextState] is equal to [state] - which is the value that will be returned.
Map get nextState => _nextState == null ? state : _nextState;

/// Reference to the value of [props] for the upcoming render cycle.
///
/// Used internally for proxying ReactJS lifecycle methods [shouldComponentUpdate], [componentWillReceiveProps], and [componentWillUpdate].
/// Used internally for proxying ReactJS lifecycle methods [shouldComponentUpdate], [componentWillReceiveProps], and
/// [componentWillUpdate] as well as the context-specific variants.
///
/// __DO NOT set__ from anywhere outside react-dart lifecycle internals.
Map nextProps;

/// Transfers `Component` [_nextContext] to [context], and [context] to [prevContext].
void transferComponentContext() {
prevContext = context;
if (_nextContext != null) {
context = _nextContext;
}
_nextContext = new Map.from(context);
}

/// Transfers `Component` [_nextState] to [state], and [state] to [prevState].
void transferComponentState() {
prevState = state;
Expand Down Expand Up @@ -217,37 +251,102 @@ abstract class Component {
///
/// Calling [setState] within this function will not trigger an additional [render].
///
/// __Note__: Choose either this method or [componentWillReceivePropsWithContext]. They are both called at the same
/// time so using both provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentwillreceiveprops>
void componentWillReceiveProps(Map newProps) {}

/// ReactJS lifecycle method that is invoked when a `Component` is receiving [newProps].
///
/// This method is not called for the initial [render].
///
/// Use this as an opportunity to react to a prop or context transition before [render] is called by updating the
/// [state] using [setState]. The old props and context can be accessed via [props] and [context], respectively.
///
/// Calling [setState] within this function will not trigger an additional [render].
///
/// __Note__: Choose either this method or [componentWillReceiveProps]. They are both called at the same time so using
/// both provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentwillreceiveprops>
void componentWillReceivePropsWithContext(Map newProps, nextContext) {}

/// ReactJS lifecycle method that is invoked before rendering when [nextProps] or [nextState] are being received.
///
/// Use this as an opportunity to return false when you're certain that the transition to the new props and state
/// Use this as an opportunity to return `false` when you're certain that the transition to the new props and state
/// will not require a component update.
///
/// __Note__: This method is called after [shouldComponentUpdateWithContext]. When it returns `null`, the result of
/// this method is used, but this is not called if a valid `bool` is returned from [shouldComponentUpdateWithContext].
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-shouldcomponentupdate>
bool shouldComponentUpdate(Map nextProps, Map nextState) => true;

/// ReactJS lifecycle method that is invoked before rendering when [nextProps], [nextState], or [nextContext] are
/// being received.
///
/// Use this as an opportunity to return `false` when you're certain that the transition to the new props, state, and
/// context will not require a component update.
///
/// __Note__: This method is called before [shouldComponentUpdate]. Returning `null` will defer the update to the
/// result of [shouldComponentUpdate], but [shouldComponentUpdate] is not called if a valid `bool` is returned.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-shouldcomponentupdate>
bool shouldComponentUpdateWithContext(Map nextProps, Map nextState, Map nextContext) => null;

/// ReactJS lifecycle method that is invoked immediately before rendering when [nextProps] or [nextState] are being
/// received.
///
/// This method is not called for the initial [render].
///
/// Use this as an opportunity to perform preparation before an update occurs.
///
/// __Note__: Choose either this method or [componentWillUpdateWithContext]. They are both called at the same time so
/// using both provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentwillupdate>
void componentWillUpdate(Map nextProps, Map nextState) {}

/// ReactJS lifecycle method that is invoked immediately before rendering when [nextProps], [nextState], or
/// [nextContext] are being received.
///
/// This method is not called for the initial [render].
///
/// Use this as an opportunity to perform preparation before an update occurs.
///
/// __Note__: Choose either this method or [componentWillUpdate]. They are both called at the same time so using both
/// provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentwillupdate>
void componentWillUpdateWithContext(Map nextProps, Map nextState, Map nextContext) {}

/// ReactJS lifecycle method that is invoked immediately after the `Component`'s updates are flushed to the DOM.
///
/// This method is not called for the initial [render].
///
/// Use this as an opportunity to operate on the [rootNode] (DOM) when the `Component` has been updated as a result
/// of the values of [prevProps] / [prevState].
///
/// __Note__: Choose either this method or [componentDidUpdateWithContext]. They are both called at the same time so
/// using both provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentdidupdate>
void componentDidUpdate(Map prevProps, Map prevState) {}

/// ReactJS lifecycle method that is invoked immediately after the `Component`'s updates are flushed to the DOM.
///
/// This method is not called for the initial [render].
///
/// Use this as an opportunity to operate on the [rootNode] (DOM) when the `Component` has been updated as a result
/// of the values of [prevProps] / [prevState] /[prevContext].
///
/// __Note__: Choose either this method or [componentDidUpdate]. They are both called at the same time so using both
/// provides no added benefit.
///
/// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentdidupdate>
void componentDidUpdateWithContext(Map prevProps, Map prevState, Map prevContext) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since React 16 doesn't include prevContext in componentDidUpdate, can we just omit this lifecycle method?


/// ReactJS lifecycle method that is invoked immediately before a `Component` is unmounted from the DOM.
///
/// Perform any necessary cleanup in this method, such as invalidating timers or cleaning up any DOM [Element]s that
Expand Down
36 changes: 30 additions & 6 deletions lib/react_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ final ReactDartInteropStatics _dartInteropStatics = (() {
void handleComponentWillMount(Component component) => zone.run(() {
component
..componentWillMount()
..transferComponentState();
..transferComponentState()
..transferComponentContext();
});

/// Wrapper for [Component.componentDidMount].
Expand All @@ -249,12 +250,15 @@ final ReactDartInteropStatics _dartInteropStatics = (() {
/// 1. Update [Component.props] using the value stored to [Component.nextProps]
/// in `componentWillReceiveProps`.
/// 2. Update [Component.state] by calling [Component.transferComponentState]
/// 3. Update [Component.context] with the latest
/// 3. Update [Component.context] by calling [Component.transferComponentContext]
void _afterPropsChange(Component component, InteropContextValue nextContext) {
component.props = component.nextProps; // [1]
component.transferComponentState(); // [2]
// [3]
component.context = _unjsifyContext(nextContext);
component.transferComponentContext(); // [3]
Copy link
Collaborator

Choose a reason for hiding this comment

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

From this code, and from testing the example, context doesn't seem to ever get updated.

nextState gets updated in setState and then transferred to state in transferComponentContext, but there doesn't seem to be an analagous updating of nextContext here.

I would recommend updating how context is managed to mirror props as opposed to state, since they both are updated externally.

This updating should also have test coverage (see later comment in tests).

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 believe that is a react-js issue: facebook/react#2517.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that issue is around how to handle context not getting updated when shouldComponentUpdate returns false.

Otherwise, context should update.

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 agree. I'm validating that. The initial code had this issue and I found several references of it not updating.

I'll ping back once I confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Built a react-js app and it does indeed update. Investigating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok...it is refactored to match the props integration.

}

void _clearPrevContext(Component component) {
component.prevContext = null;
}

void _clearPrevState(Component component) {
Expand Down Expand Up @@ -286,21 +290,35 @@ final ReactDartInteropStatics _dartInteropStatics = (() {
bool handleShouldComponentUpdate(Component component, InteropContextValue nextContext) => zone.run(() {
_callSetStateTransactionalCallbacks(component);

if (component.shouldComponentUpdate(component.nextProps, component.nextState)) {
// If shouldComponentUpdateWithContext returns a valid bool (default implementation returns null),
// then don't bother calling `shouldComponentUpdate` and have it trump.
bool shouldUpdate =
component.shouldComponentUpdateWithContext(component.nextProps, component.nextState, component.nextContext);

if (shouldUpdate == null) {
shouldUpdate = component.shouldComponentUpdate(component.nextProps, component.nextState);
}

if (shouldUpdate) {
return true;
} else {
// If component should not update, update props / transfer state because componentWillUpdate will not be called.
_afterPropsChange(component, nextContext);
_callSetStateCallbacks(component);
// Clear out prevState after it's done being used so it's not retained
_clearPrevState(component);
// Clear out prevContext after it's done being used so it's not retained
_clearPrevContext(component);
return false;
}
}
});

/// Wrapper for [Component.componentWillUpdate].
void handleComponentWillUpdate(Component component, InteropContextValue nextContext) => zone.run(() {
/// Call `componentWillUpdate` and the context variant
component.componentWillUpdate(component.nextProps, component.nextState);
component.componentWillUpdateWithContext(component.nextProps, component.nextState, component.nextContext);

_afterPropsChange(component, nextContext);
});

Expand All @@ -309,10 +327,16 @@ final ReactDartInteropStatics _dartInteropStatics = (() {
/// Uses [prevState] which was transferred from [Component.nextState] in [componentWillUpdate].
void handleComponentDidUpdate(Component component, ReactDartComponentInternal prevInternal) => zone.run(() {
var prevInternalProps = prevInternal.props;

/// Call `componentDidUpdate` and the context variant
component.componentDidUpdate(prevInternalProps, component.prevState);
component.componentDidUpdateWithContext(prevInternalProps, component.prevState, component.prevContext);

_callSetStateCallbacks(component);
// Clear out prevState after it's done being used so it's not retained
_clearPrevState(component);
// Clear out prevContext after it's done being used so it's not retained
_clearPrevContext(component);
});

/// Wrapper for [Component.componentWillUnmount].
Expand Down