Add forwardRef2/memo2 which fixes jsification of Dart props by greglittlefield-wf · Pull Request #275 · Workiva/react-dart

greglittlefield-wf · 2020-08-13T19:15:13Z

Motivation

Dart forwardRef components and memo-wrapped components were being passed JSified props due to the use of JS component factories, when they expected to be getting non-converted Dart props.

Solution

tl;dr: I added forwardRef2/memo2, added a bunch of tests around refs, and fixed a bunch of issues surfaced by test failures.

Add forwardRef2 and memo2, which return Dart component factories so they don't convert props
- I had to make new versions because fixing the issues required return type changes that would be breaking. We can consume these new versions under the hood in over_react's memo/uiForwardRef without any breakages.
- Remove prop de-converting logic from forwardRef2
  This was only a partial solution to wrapping JS components, only applying to callbacks
  
  Also, now that we're using Dart component factories, this logic would prevent Dart event handlers from being converted.
  
  So, since we'll need a more holistic solution to the problem of wrapping JS components, and it wouldn't be easy to retain this logic without breaking other behavior, it was removed.
Update common factory tests:
- Refactor simple ref tests use ref test case objects for consistency and to DRY up setup
- Add tests for all Dart factories (including forwardRef2/memo2) that Dart props are passed in without being converted
  These verify that the main issue that motivated this PR (converted JS props being passed to Dart components) is fixed
- Add tests that verify that all ref types work with forwardRef
- Add tests that chainRef works with refs that come from forwardRef2 callback args (as well as ReactElement, since it experienced a similar problem)
  - This required some code changes in different places to get these passing
- Add test cases JS ref objects are populated with the correct values
Update event handler tests to not test de-conversion of props via forwardRef

QA Instructions

Verify unit tests pass (CI passes)
Verify the function components in WSD that originally surfaced these issues still work (@aaronlademann-wf)
Smoke test examples
Perform QA instructions in over_react PR consuming these in uiForwardRef and memo: CPLAT-11977 Consume/update typing for forwardRef2/memo2 functions over_react#620

… later if we need to

greglittlefield-wf · 2020-08-13T22:17:42Z

lib/react_client/component_factory.dart

  JsFunctionComponent get type => reactFunction;
+}

-  static String _getJsFunctionName(Function object) =>


Use whitespace-agnostic diff for this section

greglittlefield-wf · 2020-08-13T22:19:50Z

test/lifecycle_test.dart

        });
      });
-
-      group('props are received correctly without interop interfering with the values:', () {


These were moved to the common factory tests so they run for all Dart components and Dart component HOCs, not just Dart function components

This reverts commit 3dacc70.

lib/react.dart

aaronlademann-wf

@greglittlefield-wf this is awesome work!

Found a couple of small things.

Also, do we have a test for the case where a memo2 wraps a forwardRef2? I know we loosened the typing on memo2 to allow for it - but I don't think I saw a test asserting that it works. Would be nice to have to prevent future regressions.

test/factory/common_factory_tests.dart

aaronlademann-wf · 2020-08-21T13:05:44Z

test/factory/common_factory_tests.dart

    });

+    group('when refs come from sources where they have been potentially converted:', () {
+      test('ReactElement.ref', () {


test/factory/dart_function_factory_test.dart

test/forward_ref_test.dart

test/factory/dart_function_factory_test.dart

test/react_memo_test.dart

greglittlefield-wf · 2020-09-04T00:06:31Z

@aaronlademann-wf @smaifullerton-wk All feedback has been addressed! (including adding a test case for wrapping forwardRef2 with memo)

greglittlefield-wf · 2020-09-04T00:07:44Z

test/react_memo_test.dart

        reason: 'test setup sanity check');
  }

-  group('memo', () {


Best reviewed with whitespace-agnostic diff; this actually didn't change, but was just moved into a function

aaronlademann-wf

+1

example/test/function_component_test.dart

…rdRef2 and test all ref types

# Conflicts: # lib/hooks.dart # test/hooks_test.dart

greglittlefield-wf · 2020-09-08T21:18:12Z

test/hooks_test.dart

    });

    group('useImperativeHandle -', () {
-      var mountNode = new DivElement();


Best viewed with whitespace-agnostic diff; besides changing to forwardRef2, these were just indented

greglittlefield-wf · 2020-09-08T21:20:06Z

lib/src/react_client/dart_interop_statics.dart

+          if (ref is ReactComponent) return ref.dartComponent ?? ref;

-          return (ref as ReactComponent).dartComponent ?? ref;
+          return ref;


These changes were to fix an error when setting a string ref on a component using useImperativeHandle (because it would try to cast the customized ref to ReactComponent).

This an edge-case, but it was easier to fix the issue than to skip testing that case.

test/hooks_test.dart

sydneyjodon-wk · 2020-09-08T22:21:48Z

QAing now... I think this just needs to be formatted?

greglittlefield-wf · 2020-09-09T21:44:28Z

.travis.yml

-  - dartfmt --line-length=120 --dry-run --set-exit-if-changed .
+  - |
+    if [ "$TRAVIS_DART_VERSION" != "dev" ]; then
+      dartfmt --line-length=120 --dry-run --set-exit-if-changed .


This fixes failures we were seeing in the Dart dev builds due to the dartfmt being different

sydneyjodon-wk · 2020-09-09T22:55:06Z

Verify unit tests pass (CI passes)

Smoke test examples

Perform QA instructions in over_react PR

QA+1 for every step except:

Verify the function components in WSD that originally surfaced these issues still work (@aaronlademann-wf )

aaronlademann-wf · 2020-09-10T15:01:35Z

.travis.yml

  - dartanalyzer .
-  - dartfmt --line-length=120 --dry-run --set-exit-if-changed .
+  - |
+    if [ "$TRAVIS_DART_VERSION" != "dev" ]; then


aaronlademann-wf · 2020-09-10T15:02:29Z

QA +1

Verify the function components in WSD that originally surfaced these issues still work

greglittlefield-wf added 9 commits August 13, 2020 12:14

Add forwardRef2 which fixes jsification of Dart props

b4cb55f

Export forwardRef2

c0fb93e

Add missing call to JS forwardRef

7b8fb0e

Add conditional ref logic

6256901

Add tests for prop values and forwardRef2

9f1aade

Fix memo, add memo2

02a9aea

Format

8468f54

Make return typing less specific so we can change the underlying type…

0617c3a

… later if we need to

Accept any factory type in memo2

d3036fd

greglittlefield-wf changed the title ~~Add forwardRef2 which fixes jsification of Dart props~~ Add forwardRef2/memo2 which fixes jsification of Dart props Aug 13, 2020

greglittlefield-wf commented Aug 13, 2020

View reviewed changes

greglittlefield-wf and others added 9 commits August 13, 2020 15:29

Remove unused arg

54b21e9

Fix deprecation warnings, test memo2

490d7e5

Test forwardRef2

c65febe

Move forwardRef tests into their own file, fix naming tests

66e3bed

Fix forwardRef2 ref typing

b202ca0

Remove redundant tests covered by commonFactoryTests

3dacc70

Fix ref wrapping test

e9e62e9

Use forwardRef2 in examples

ff46864

Make memo work around forwardRef

4a0cbfb

greglittlefield-wf force-pushed the forwardRef-fix branch from 72088b7 to 4a0cbfb Compare August 14, 2020 19:13

greglittlefield-wf added 8 commits August 14, 2020 13:25

Revert "Remove redundant tests covered by commonFactoryTests"

fdfbd53

This reverts commit 3dacc70.

Make handler wrapping test more generic

b344b1b

Remove unnecessary isRawJsFunctionFromProps logic

9e70ee0

Add ref unconversion utils and consume in forwardRef2

fda51a4

Relocate similar event handler unconversion logic

ccc1ad8

Improve forwardRef and base ref tests, fix chaining of converted refs

d2d04d2

Clean up test case objects

874ccd6

Fix lints

450ecf1

greglittlefield-wf requested review from aaronlademann-wf, joebingham-wk, smaifullerton-wk, sydneyjodon-wk and willdrach-wk August 20, 2020 22:11

greglittlefield-wf mentioned this pull request Aug 20, 2020

CPLAT-11977 Consume/update typing for forwardRef2/memo2 functions Workiva/over_react#620

Merged

7 tasks

smaifullerton-wk reviewed Aug 20, 2020

View reviewed changes

lib/react.dart Outdated Show resolved Hide resolved

aaronlademann-wf requested changes Aug 21, 2020

View reviewed changes

greglittlefield-wf added 3 commits September 3, 2020 16:23

Address CR feedback related to comments

70ebf83

Reinstate tests for memo and forwardRef

8d52aa7

Add regression test for passing forwardRef2 into memo2

fe266b9

greglittlefield-wf commented Sep 4, 2020

View reviewed changes

aaronlademann-wf approved these changes Sep 8, 2020

View reviewed changes

sydneyjodon-wk reviewed Sep 8, 2020

View reviewed changes

example/test/function_component_test.dart Show resolved Hide resolved

greglittlefield-wf added 2 commits September 8, 2020 14:13

Fix forwardRef2/useImperativeHandle typing, update tests to use forwa…

be896a7

…rdRef2 and test all ref types

Merge remote-tracking branch 'origin/master' into forwardRef-fix

1d3fea5

# Conflicts: # lib/hooks.dart # test/hooks_test.dart

greglittlefield-wf commented Sep 8, 2020

View reviewed changes

test/hooks_test.dart Outdated Show resolved Hide resolved

Remove print statement

3bbdff7

Don't check dartfmt when using the dev build of Dart

5807944

greglittlefield-wf commented Sep 9, 2020

View reviewed changes

aaronlademann-wf reviewed Sep 10, 2020

View reviewed changes

aaronlademann-wf merged commit f9ab447 into master Sep 10, 2020

aaronlademann-wf mentioned this pull request Sep 10, 2020

Release react 5.6.0 #278

Merged

greglittlefield-wf deleted the forwardRef-fix branch February 16, 2022 21:52

Conversation

greglittlefield-wf commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

QA Instructions

Uh oh!

greglittlefield-wf Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aaronlademann-wf Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greglittlefield-wf commented Sep 4, 2020

Uh oh!

greglittlefield-wf Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greglittlefield-wf Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sydneyjodon-wk commented Sep 8, 2020

Uh oh!

greglittlefield-wf Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

sydneyjodon-wk commented Sep 9, 2020 • edited by aaronlademann-wf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlademann-wf Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf commented Sep 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

greglittlefield-wf commented Aug 13, 2020 •

edited

Loading

greglittlefield-wf Aug 13, 2020 •

edited

Loading

greglittlefield-wf Sep 4, 2020 •

edited

Loading

sydneyjodon-wk commented Sep 9, 2020 •

edited by aaronlademann-wf

Loading