Context support #135

johnbland-wf · 2017-11-01T21:06:20Z

Feature

Support context.

Solution

Provided support for context without a breaking change by adding extra *WithContext methods. All of these methods replace the normal lifecycle methods so both are not needed.
- componentDidUpdateWithContext
- componentWillUpdateWithContext
- shouldComponentUpdateWithContext
- componentWillReceivePropsWithContext
shouldComponentUpdateWithContext returns null by default so shouldComponentUpdate is still used. If shouldComponentUpdateWithContext returns a valid bool, shouldComponentUpdate will not get checked
Updated tests to verify all of the lifecycle methods are called

Testing

Verify unit tests pass
Verify context example makes sense and works as expected

jeroencranendonk-wf · 2017-11-01T22:44:33Z

js_src/dart_helpers.js

+      config.childContextTypes[childContextKeys[i]] = React.PropTypes.object;
+    }
+
+    // Only declare this when `hasChildContext` is true to avoid unnecessarily


Referencing an old variable?

Possibly. I'll dig into it.

I think so, sorry about that. Should probably read when childContextKeys is non-empty

greglittlefield-wf · 2017-11-08T21:54:39Z

@greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf

greglittlefield-wf

Looks good, mainly just an issue around context updating and related test coverage.

So sorry it took me so long to take a look at this; I'll definitely be quicker to respond moving forward.

greglittlefield-wf · 2017-11-13T18:59:58Z

lib/react.dart

    _initProps(props);
  }

+  /// Initializes context
+  _initContext(context) {
+    this.context = new Map.from(context ?? {});


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

greglittlefield-wf · 2017-11-13T19:00:25Z

lib/react.dart

+  _initContext(context) {
+    this.context = new Map.from(context ?? {});
+
+    /// Call `transferComponentContext` to get state also to `_prevContext`


#nit This should not be a doc comment

greglittlefield-wf · 2017-11-13T19:00:28Z

lib/react.dart

  _initProps(props) {
    this.props = new Map.from(props);
    this.nextProps = this.props;
  }

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


#nit This should not be a doc comment

greglittlefield-wf · 2017-11-13T19:01:09Z

lib/react.dart

@@ -114,16 +146,31 @@ abstract class Component {

  /// Public getter for [_nextState].


s/_nextState/_nextContext/

greglittlefield-wf · 2017-11-13T19:15:27Z

lib/react_client.dart

    component.props = component.nextProps; // [1]
    component.transferComponentState();    // [2]
+    component.transferComponentContext();  // [3]


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).

I believe that is a react-js issue: facebook/react#2517.

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

Otherwise, context should update.

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.

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

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

greglittlefield-wf · 2017-11-13T19:18:36Z

lib/react.dart

+  /// provides no added benefit.
+  ///
+  /// See: <https://facebook.github.io/react/docs/react-component.html#updating-componentdidupdate>
+  void componentDidUpdateWithContext(Map prevProps, Map prevState, Map prevContext) {}


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

greglittlefield-wf · 2017-11-13T19:22:27Z

test/lifecycle_test.dart

      ]));
    });

-    group('when shouldComponentUpdate returns false:', () {
+    void testShouldUpdates({bool shouldComponentUpdateWithContext, bool shouldComponentUpdate}) {
      test('recieves updated props with correct lifecycle calls and does not rerender', () {


There should be tests that context maps are updated as expected and with the correct values in the correct lifecycle methods.

An easy way to do that would be to parallel this props update test as well as the other "recieves updated props" test.

johnbland-wf · 2017-12-01T02:50:33Z

Ok, this now updates context as expected. It isn't yet unit tested, but the big hurdle is done. Gotta jet. I'll tighten this up tomorrow.

johnbland-wf · 2017-12-01T23:34:44Z

Ok, @greglittlefield-wf...final review time.

That test found some issues w/ the wrong context being passed to lifecycle methods. Let me know how it sits.

jacehensley-wf · 2017-12-05T18:43:01Z

lib/react.dart

@@ -139,6 +172,8 @@ abstract class Component {
  ///
  /// [A.k.a "forceUpdate"](https://facebook.github.io/react/docs/react-component.html#forceupdate)
  void redraw([callback()]) {
+    context = getChildContext();


Why does this happen here but not in setState?

Hrmm...guess I mistook JS for Dart here w/ the _jsRedraw vs redraw.

Should it go in replaceState as well?

Hmm, what's the purpose of doing this anyways? Doesn't this https://github.com/cleandart/react-dart/pull/135/files#diff-12ac054bb8b76f35c1035afef1d94396R257 update the context?

Great find. I removed it and all tests pass + examples work. See 723caa5.

jacehensley-wf · 2017-12-05T19:23:07Z

+1

greglittlefield-wf

One comment on internal context management, some small stuff, and a couple areas where test coverage could be improved.

Otherwise, looks great!! This is really close!

greglittlefield-wf · 2017-12-05T20:48:58Z

lib/react.dart

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


Dangling doc comment. Looks like this should be the doc comment for nextContext whose current comment doesn't seem to match.

Nope, that was dead code from a previous build. Removing...

greglittlefield-wf · 2017-12-05T20:58:06Z

lib/react_client.dart

-  void _afterPropsChange(Component component) {
-    component.props = component.nextProps; // [1]
-    component.transferComponentState();    // [2]
+  /// 2. Update [Component.context] using the value stored to [Component.nextContext]


This comment doesn't patch up with what's being done.

Also, in handleComponentWillReceiveProps, why isn't nextContext being stored onto the component like props is? It's likely better for perf to match that pattern and only call _unjsifyContext once per rerender vs the 3 times per rerender that it's called currently.

Ah, man...too many twists and turns in here. :-D Great find. I found where the issue was w/ nextContext being outdated.

See f0d8b3a.

greglittlefield-wf · 2017-12-05T21:01:37Z

lib/react_client/react_interop.dart

@@ -178,8 +192,7 @@ class InteropProps {
  external factory InteropProps({ReactDartComponentInternal internal, String key, dynamic ref});
 }

-/// Internal react-dart information used to proxy React JS lifecycle to Dart
-/// [Component] instances.
+/// A Dart object that stores .


Looks like this was leftover from my one of my commits, sorry 😬. Can you revert this doc comment change, please?

greglittlefield-wf · 2017-12-05T21:02:42Z

test/ReactSetStateTestComponent.js

@@ -27,15 +27,28 @@ var ReactSetStateTestComponent = React.createClass({
    this.recordLifecyleCall("componentWillReceiveProps");
  },

+  componentWillReceivePropsWithContext: function(_) {


These -WithContext methods on the JS component won't ever get called, right? Why do they exist?

I believe it was only testing so recordLifecyleCall would record the call and could be validated.

Not sure I'm following. recordLifecycleCall won't be called since nothing is calling these methods on the JS component, right?

greglittlefield-wf · 2017-12-05T21:08:37Z

test/lifecycle_test.dart

+    });
+
+    group('when shouldComponentUpdateWithContext returns false:', () {
+      testShouldUpdates(shouldComponentUpdateWithContext: false, shouldComponentUpdate: false);
    });


Should we add one more test case that shouldComponentUpdate is not called when shouldComponentUpdateWithContext returns true? Or is that overkill?

That's pretty much what this is testing:

List calls = [ matchCall('componentWillReceiveProps', args: [newPropsWithDefaults], props: initialPropsWithDefaults), matchCall('componentWillReceivePropsWithContext', args: [newPropsWithDefaults, expectedContext], props: initialPropsWithDefaults), matchCall('shouldComponentUpdateWithContext', args: [newPropsWithDefaults, expectedState, expectedContext], props: initialPropsWithDefaults), ]; if (shouldComponentUpdateWithContext == null) { calls.add( matchCall('shouldComponentUpdate', args: [newPropsWithDefaults, expectedState], props: initialPropsWithDefaults), ); }

When shouldComponentUpdateWithContext is null we validate the method is called. If it was called when shouldComponentUpdateWithContext had a value, this test would fail.

Right, I just meant testing true as a value in addition to false.

greglittlefield-wf · 2017-12-05T21:10:29Z

test/lifecycle_test.dart

@@ -139,7 +140,78 @@ void main() {
      ]));
    });


There should be a lifecycle tests that asserts that getChildContext is not called when childContextKeys is empty.

Tests incoming. Good call here.

See 282914b.

greglittlefield-wf · 2017-12-05T21:12:17Z

test/lifecycle_test.dart

+        matchCall('render',                                                                                             props: newPropsWithDefaults, context: expectedContext),
+        matchCall('componentDidUpdate',                   args: [initialPropsWithDefaults, expectedState],              props: newPropsWithDefaults, context: expectedContext),
+      ]));
+    });


Awesome test! Could we add one more that verifies that only the keys present in childContextKeys are passed down, though?

Setting an extra key to pass but not having the value in the child should be enough. That's incoming.

See 282914b.

johnbland-wf · 2017-12-05T22:37:13Z

Ok, @greglittlefield-wf, I think it is ready.

Big props to you and @jacehensley-wf for catching those issues.

greglittlefield-wf · 2017-12-11T20:08:04Z

+1 looks great!

Going to pull this down and test, and also pull into some other repos for testing.

greglittlefield-wf · 2017-12-11T21:50:06Z

Context works as expected in example
All tests pass in content-shell, dart2js, and DDC
All tests pass when pulled into OverReact and WSD

+10

jacehensley-wf · 2017-12-11T21:59:22Z

+1 refresh

+ Deprecates everything that was added in Workiva#135 + New context API will be released alongside support for ReactJS 16 in version 5.0.0 of this package

greglittlefield-wf and others added 17 commits October 26, 2017 15:57

Port over initial stab at context

d2e4beb

Fix breaking change to initComponentInternal

3ec9436

Use internal pattern for individual context keys

e1a44b0

Fix JS, conditionally declare context pieces on config

27fea39

Generate JS files

2408a09

Cleanup, naming, docs

f772419

Ignore prevContext since it isn't supported in React 16

bb04f63

Generate JS

b80ac0a

Rename to handleGetChildContext

bb0bc6c

Add prev/nextContext and update docs

9989c2e

Added logging to js build

2e7d74b

Added lifecycle call checks to LifecycleTest

f605e0d

Fix errors in react_test examples

88bb52f

Fix errors in ref_test examples

b410e62

Fix errors in speed_test examples

a8585e8

Fix errors in get_dom_node_test examples

2fba623

Adjust context component to test childContextKeys

cd9ddaf

johnbland-wf mentioned this pull request Nov 1, 2017

Context support (WIP) #134

Closed

jeroencranendonk-wf reviewed Nov 1, 2017

View reviewed changes

Update dart_helpers comment

846c0f1

greglittlefield-wf requested changes Nov 13, 2017

View reviewed changes

johnbland-wf added 8 commits November 30, 2017 17:22

Merge branch 'master' of github.com:cleandart/react-dart into context

987b995

Clean-up / Removed componentDidUpdateWithContext

6e31305

Added testing steps to README

61c6609

Update example to showcase state updates

7861c20

Use context directly vs transferring context

804ac5e

Add JS build to README

64fbaae

recieves -> receives

5f4d403

Updated example component to show context updates to grandchildren

dc0e5af

Fix lifecycle calls and add test coverage

92fbc53

jacehensley-wf reviewed Dec 5, 2017

View reviewed changes

Remove forced context update

723caa5

greglittlefield-wf requested changes Dec 5, 2017

View reviewed changes

johnbland-wf added 4 commits December 5, 2017 15:58

Documentation updates

f791d0d

Use nextContext vs unjsify’ing it

f0d8b3a

Test updates

282914b

Remove context test methods in JS

3602963

greglittlefield-wf approved these changes Dec 11, 2017

View reviewed changes

greglittlefield-wf merged commit 84530cd into Workiva:master Dec 14, 2017

johnbland-wf deleted the context branch December 14, 2017 19:50

		@@ -114,16 +146,31 @@ abstract class Component {

		/// Public getter for [_nextState].

Context support #135

Context support #135

Conversation

johnbland-wf commented Nov 1, 2017

Feature

Solution

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf commented Nov 8, 2017

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbland-wf commented Dec 1, 2017

johnbland-wf commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Dec 5, 2017

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbland-wf Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbland-wf commented Dec 5, 2017

greglittlefield-wf commented Dec 11, 2017

greglittlefield-wf commented Dec 11, 2017

jacehensley-wf commented Dec 11, 2017

johnbland-wf Dec 5, 2017 •

edited

Loading