CPLAT-4303: React 16 New Context #160

kealjones-wk · 2019-02-28T00:03:17Z

We may delay this PR until we start work on ComponentV2 and then implement it in ComponentV2 and put Legacy Context back in Component but this pr is ready for an implementation CR with this in mind.

Merge conflicts are to be expected in the lib folders .js and .map files these are generated from the js_src files and running npm run build. They will be resolved with a merge of the js_src folders and a new build on the wip branch.

Description:

React 16 implements a better New Context API.

Changes:

Added createContext to react_client.dart
Added contextType to react.Component2
Added ReactDartContext that utilize interop ReactContext for use in ReactJS
Added ReactJsComponentFactoryProxy that was originally created by @greglittlefield-wf about 2 years ago, then added a ReactJsContextComponentFactoryProxy for use with react16 and context components.
- Resurrects Support component classes declared in JS #112 to support Use JS React components from Dart (e.g., Material-UI) #109
Add tests for ReactJsComponentFactoryProxy
Changes type checking test that was used for JsBackedMaps to a shared suite, which was then also used for context.
Updated tests that previously skipped thought to be deprecated lifecycle method shouldComponentUpdateWithContext
Add tests for calculateChangedBits argument of context.
Update getSnapshotBeforeUpdate tests to use new shared type tests
Removes js_util deprecations and todos from Deprecate Original JS Interop Helpers CPLAT-4909 #171

Testing suggestions:

CI Passes
run webdev serve example and open http://localhost:8080
- check http://localhost:8080/test/context_test.html and make sure it works as expected
- check http://localhost:8080/js_components/index.html and make sure it works as expected

Potential areas of regression:

# Conflicts: # test/js_interop_helpers_test/shared_tests.dart

* master: Bump version meta for 4.6.1 release Use isInstanceOf instead of TypeMatcher widen build package ranges type style map

aaronlademann-wf

@kealjones-wk didn't make it all the way through, have a parent teacher conference for Noah this morning at 8:30. Will continue taking a look when I return.

lib/react_client.dart

example/js_components/js_components.dart

aaronlademann-wf · 2019-03-21T14:30:54Z

example/test/react_test_components.dart

+  if (nextValue['renderCount'] % 2 == 0) {
+    result |= 1 << 2;
+  }
+  return result;


I have literally no idea what is happening, or why it is happening. The second argument of createContext deserves some documentation explaining what it does. For instance, why does it always return an integer?

The React team doesn't even have documentation around it. They act as if it doesn't exist even though it obviously does, i think this may still be a WIP/secret or experimental feature...

The idea is that you return a bit that indicates which parts/or how the context value changed, then you can use the observedBits props on a consumer and they will only re-render if the bits they are observing are returned from calculateChangedBits

Sorry for the crazy delayed reply here @kealjones-wk - a doc comment about how you perceive it to work (like what you just typed above) would be a great addition!

aaronlademann-wf · 2019-03-21T14:33:30Z

example/test/react_test_components.dart

+
+class _NewContextConsumerComponent extends react.Component2 {
+  render() {
+    return TestNewContext.Consumer({'unstable_observedBits': props['unstable_observedBits']}, (value) {


Why are we using unstable ReactJS APIs in something extending from Component2?

This is just an example but, the unstable_observedBits prop functions better than the stable version of the prop: observedBits... thats why its used here

Sorry for the delayed reply.

I get that its just an example... but it is an example intended to exercise the functionality in-lieu of a formal functional test - which is why I'm concerned about our implementation possibly not functioning well with the stable version of observedBits.

example/test/react_test_components.dart

js_src/_dart_helpers.js

lib/react.dart

lib/react_client.dart

lib/src/react_client/context.dart

test/factory/common_factory_tests.dart

js_src/_dart_helpers.js

lib/react.dart

test/lifecycle_test.dart

test/lifecycle_test/component2.dart

tainhenning-wk · 2019-04-01T22:02:13Z

test/react_client/js_backed_map_test.dart

@@ -23,181 +24,17 @@ main() {
      JsBackedMap jsBackedMap;
      JsBackedMap dynamicJsBackedMap;

-      setUp(() {
+      testTypeValue(testValue) {


The type information of testValue is no longer being propagated. Are we sure that this test is equivalent to the old one? @greglittlefield-wf

I was thinking about this and we may be able to use generics to propagate the static type information, and use JS inlining annotations from package:meta to force dart2js to use that type info (if it doesn't already):

Suggested change

testTypeValue(testValue) {

@tryInline

void testTypeValue<T>(T testValue) {

I'll give that a shot locally.

Related: we should update the builder config for this repo to use the Dart 2 equivalent of the trust-type-annotations flag, since that's what we'll be using in prod most of the time.

It looks manually inlining doesn't do much for the dart2js output, but we should add the generics so that both tests aren't essentially just as dynamic.

I'll make a PR to add those and also add the build.yaml and test changes

greglittlefield-wf · 2019-04-02T17:17:12Z

test/shared_type_tester.dart

+import 'package:js/js.dart';
+import 'package:test/test.dart';
+
+sharedTypeTests(Function testTypeValue,


This typing should be more specific:

Suggested change

sharedTypeTests(Function testTypeValue,

typedef void TestTypeValue<T>(T value);

void sharedTypeTests(TestTypeValue testTypeValue,

We should avoid unnecessary typedefs since we're Dart 2 only now... we can just use the new inline Function syntax.

Good point; the only reason I put in the typedef was because I wasn't sure if the semantics were the same for generics

Addressed in kealjones-wk#3

test/shared_type_tester.dart

test/react_context_test.dart

lib/react.dart

greglittlefield-wf · 2019-04-02T17:50:06Z

test/react_context_test.dart

+    expect(consumerRef.latestValue, same(typeToTest));
+  }
+
+  group('New Context API (Component2 only)', () {


Are there any tests that the correct nextContext arg is passed into shouldComponentUpdate? I can't seem to find any. If not, could you add some?

lib/react.dart

React 16 new context

aaronlademann-wf

+1

Really nice work on this @kealjones-wk !!!

greglittlefield-wf

Some things we should address in a follow-up PR, but are good for now:

Comments in this review

Comment about testing the nextContext value passed into shouldComponentUpdate (the only test we have uses null; we should also have a test with another value)
The JS components demo fails in release mode:

js_components.dart.js:9006 Uncaught ReferenceError: ui is not defined
    at js_components.dart.js:9006
    at Object.holder.(anonymous function) (http://localhost:8080/js_components/js_components.dart.js:105:35)
    at _IndexComponent.render$0 (js_components.dart.js:8492)
    at _handleRender_closure.call$0 (js_components.dart.js:8249)
    at _RootZone.run$1$1 (js_components.dart.js:5644)
    at _RootZone.run$1 (js_components.dart.js:5648)
    at closure_handleRender.call$4 (js_components.dart.js:8240)
    at Object.Primitives_applyFunction (js_components.dart.js:457)
    at Object.Function_apply (js_components.dart.js:2451)
    at _callDartFunctionFast (js_components.dart.js:2791)

This is awesome!!!

+10

greglittlefield-wf · 2019-04-03T19:38:58Z

js_src/_dart_helpers.js

-
+/// Prefix to namespace react-dart symbols
+var _reactDartSymbolPrefix = 'react-dart.';
+/// A global symbol to identify javascript objects owned by react-dart context,


#nit

Suggested change

/// A global symbol to identify javascript objects owned by react-dart context,

/// A symbol to identify JavaScript objects owned by react-dart context,

greglittlefield-wf · 2019-04-03T19:39:55Z

lib/react_client.dart

  }

-  bool handleShouldComponentUpdate(Component2 component, JsMap jsNextProps, JsMap jsNextState) => zone.run(() {
+  bool handleShouldComponentUpdate(Component2 component, JsMap jsNextProps, JsMap jsNextState,
+          [dynamic jsNextContext]) =>


This shouldn't be optional, right?

greglittlefield-wf · 2019-04-03T19:40:08Z

lib/react_client.dart

@@ -587,25 +612,117 @@ ReactDartComponentFactoryProxy _registerComponent(ComponentFactory componentFact
  return new ReactDartComponentFactoryProxy(reactComponentClass);
 }

+class _ReactJsContextComponentFactoryProxy extends ReactJsComponentFactoryProxy {


This could use a doc comment

addressed

kealjones-wk · 2019-04-03T20:38:37Z

Comment about testing the nextContext value passed into shouldComponentUpdate (the only test we have uses null; we should also have a test with another value)

@greglittlefield-wf if the only test you're referring to is this one: https://github.com/cleandart/react-dart/pull/160/files#diff-c00a02aa016b25a86fb30a2093f935a1R520

because that component it is a child of a context.consumer callback and not a consumer/contextType component itself it will actually never have a this.context the only time this.context ever has a value is when contextType is set, and when contextType is set context and prop updates don't trigger shouldComponentUpdate.

The only time i have seen shouldComponentUpdate and nextContext be called and have a value is on a component using contextType that has a state change. But this nextContext is no different than this.context at this point so its utterly useless. but we could totally add another test to test that really awkward setup to make sure it's working.

greglittlefield-wf · 2019-04-03T22:07:11Z

But this nextContext is no different than this.context at this point so its utterly useless.

Yeah, I think we just need to verify that when it is passed in, that it's valid.

greglittlefield-wf and others added 30 commits December 29, 2016 16:29

Deprecate js_interop_helpers in favor of package:js/js_util.dart

f84c15d

Un-deprecate jsify, fix deprecations

0713393

Bump JS package version to the one that exposes js_util.dart

77f9b91

Revert likely breaking change to emptyJsMap and deprecate it instead

bfc53e5

Merge branch 'use_js_util' into js_components

68008b4

# Conflicts: # test/js_interop_helpers_test/shared_tests.dart

Add common factory tests for DOM/Dart components

4a16ba4

Add factory proxy for JS components

c174de2

Add JS factory tests

86913c2

Add jsifyAndAllowInterop, deprecate jsify

f93f521

Use jsifyAndAllowInterop

c346708

Clean up simulators slightly

f20c287

By default, convert DOM props passed to JS components

1a7b5ff

Throw if backing JS class is null

e107f93

Add common event handler tests

992586e

Add wrapEventHandler convenience method

cd9d840

Add demo on using JS components

f0c12fe

Add index to example directory

56b1d4b

Add notice about seeing Dart source

63ab345

Include full dart-lang/sdk license text in attribution

e7209e6

Merge remote-tracking branch 'origin/master' into js_components

273b915

Fix lints

8f50741

Post-merge fixes

dd34bad

dartfmt

0f38687

Fix example

e611912

inital pass at new context

2a44115

Fix js_factory_test.js div creation

c358278

ReactJsComponentFactoryProxy is WORKING!

5a8768d

Merge branch 'master' into react-16-new-context

9967b32

* master: Bump version meta for 4.6.1 release Use isInstanceOf instead of TypeMatcher widen build package ranges type style map

Fix contextType in release mode and Update tests

785615e

Update contextType to getter/setter to get doc comment to showing

06ebacf

aaronlademann-wf requested changes Mar 21, 2019

View reviewed changes

kealjones-wk added 4 commits March 22, 2019 14:50

Update Js Example to use some Material UI Components

e26827d

Address some CR feedback by aaron

745a0b7

address more feedback

71bf2ca

add small comments to example

34b95cc

kealjones-wk mentioned this pull request Apr 1, 2019

CPLAT-3094 Lifecycle Error Boundary #175

Merged

3 tasks

tainhenning-wk previously requested changes Apr 1, 2019

View reviewed changes

Address CR Feedback

f3ad312

greglittlefield-wf requested changes Apr 2, 2019

View reviewed changes

address cr and Add contextType Lifecycle tests

d38c570

aaronlademann-wf reviewed Apr 3, 2019

View reviewed changes

lib/react.dart Outdated Show resolved Hide resolved

greglittlefield-wf added 2 commits April 3, 2019 10:57

Update build config for dart2js, use build_runner to run dart2js tests

45ed03a

Update typing, remove redundant as dynamic JsBackedMap case

bb67481

greglittlefield-wf force-pushed the react-16-new-context branch from bb67481 to d38c570 Compare April 3, 2019 18:04

kealjones-wk and others added 3 commits April 3, 2019 11:59

add a context.consumer child lifecycle test

bd28d30

Merge pull request #3 from greglittlefield-wf/react-16-new-context

81dc26a

React 16 new context

dartfmt

9d6045b

aaronlademann-wf approved these changes Apr 3, 2019

View reviewed changes

greglittlefield-wf reviewed Apr 3, 2019

View reviewed changes

greglittlefield-wf approved these changes Apr 3, 2019

View reviewed changes

greglittlefield-wf merged commit 7c14909 into Workiva:5.0.0-wip Apr 3, 2019

aaronlademann-wf added this to the 5.0.0 milestone Apr 4, 2019

aaronlademann-wf modified the milestones: 5.1.0, 5.0.0 May 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-4303: React 16 New Context #160

CPLAT-4303: React 16 New Context #160

kealjones-wk commented Feb 28, 2019 •

edited by aaronlademann-wf

Loading

aaronlademann-wf left a comment

aaronlademann-wf Mar 21, 2019

kealjones-wk Mar 21, 2019

aaronlademann-wf Apr 3, 2019

aaronlademann-wf Mar 21, 2019

kealjones-wk Mar 21, 2019

aaronlademann-wf Apr 3, 2019

tainhenning-wk Apr 1, 2019

greglittlefield-wf Apr 2, 2019

greglittlefield-wf Apr 3, 2019

greglittlefield-wf Apr 2, 2019

aaronlademann-wf Apr 3, 2019

greglittlefield-wf Apr 3, 2019

greglittlefield-wf Apr 3, 2019

greglittlefield-wf Apr 2, 2019

aaronlademann-wf left a comment

greglittlefield-wf left a comment

greglittlefield-wf Apr 3, 2019

greglittlefield-wf Apr 3, 2019

greglittlefield-wf Apr 3, 2019

kealjones-wk commented Apr 3, 2019 •

edited

Loading

greglittlefield-wf commented Apr 3, 2019

	testTypeValue(testValue) {
	@tryInline
	void testTypeValue<T>(T testValue) {

	sharedTypeTests(Function testTypeValue,
	typedef void TestTypeValue<T>(T value);
	void sharedTypeTests(TestTypeValue testTypeValue,

	/// A global symbol to identify javascript objects owned by react-dart context,
	/// A symbol to identify JavaScript objects owned by react-dart context,

CPLAT-4303: React 16 New Context #160

CPLAT-4303: React 16 New Context #160

Conversation

kealjones-wk commented Feb 28, 2019 • edited by aaronlademann-wf Loading

Description:

Changes:

Testing suggestions:

Potential areas of regression:

aaronlademann-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

aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

kealjones-wk commented Apr 3, 2019 • edited Loading

greglittlefield-wf commented Apr 3, 2019

kealjones-wk commented Feb 28, 2019 •

edited by aaronlademann-wf

Loading

kealjones-wk commented Apr 3, 2019 •

edited

Loading