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.

Sign up for GitHub

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

Jump to bottom

CPLAT-8665 Expose React Hooks #611

Merged

greglittlefield-wf merged 11 commits into function-components-wip from CPLAT-8665-react-hooks

Jul 29, 2020

Contributor

sydneyjodon-wk commented Jul 25, 2020 •

edited

Loading

Motivation

React hooks were implemented in react-dart and we need to expose them in over_react.

Changes

Expose and export hooks.
Update doc comment examples to over_react from react-dart examples.
Add hooks to example.

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
  - CI passes
  - run pbr serve example and verify that examples work in http://localhost:8080/hooks/
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

sydneyjodon-wk added 3 commits

July 24, 2020 15:26


          Add the first few hooks

4aa280b


          Merge branch 'CPLAT-3873-function-components' into CPLAT-8665-react-h…

4093fe2

…ooks


          Expose the rest of the hooks

b934fb3

rmconsole-wf added the Merge Requirements Unmet label

aviary-wf commented Jul 25, 2020 •

edited

Loading

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

sydneyjodon-wk commented

View reviewed changes

example/hooks/function_components.dart Outdated

+              );
+              //final UseContextTest = uiFunction<UiProps>((props) {
+              //  final context = useContext(TestNewContext);

Contributor Author

sydneyjodon-wk Jul 25, 2020

I'm not sure if this is part of this ticket, but this example doesn't work. I think it has something to do with the react-dart version of useContext returning React.context and here it returns the over_react version of Context. It also could have to do with me not knowing much about how Context works and converting the react-dart example wrong.

Contributor

greglittlefield-wf Jul 27, 2020

I was able to get to the bottom of this, I think! Looks like there was some missing typing on some of the calls into react-dart's context, unrelated to hooks.

The following changes got it working for me:

diff --git a/lib/src/util/context.dart b/lib/src/util/context.dart
index 40f7e7c5..2299e74f 100644
--- a/lib/src/util/context.dart
+++ b/lib/src/util/context.dart
@@ -49,7 +49,7 @@ class Context<TValue> {
   Context(this.Provider, this.Consumer, this.reactDartContext);
 
   /// The react.dart version of this context.
-  final react.Context reactDartContext;
+  final react.Context<TValue> reactDartContext;
 
   /// The react.js version of this context.
   ReactContext get jsThis => reactDartContext.jsThis;
@@ -237,7 +237,7 @@ class _DO_NOT_USE_OR_YOU_WILL_BE_FIRED {
 ///
 /// Learn more: <https://reactjs.org/docs/context.html#reactcreatecontext>
 Context<TValue> createContext<TValue>([TValue defaultValue, int Function(TValue, TValue) calculateChangedBits]) {
-  react.Context reactDartContext = react.createContext(defaultValue, calculateChangedBits != null ? (dynamic arg1, dynamic arg2) => calculateChangedBits(arg1, arg2) : null);
+  final reactDartContext = react.createContext<TValue>(defaultValue, calculateChangedBits != null ? (dynamic arg1, dynamic arg2) => calculateChangedBits(arg1, arg2) : null);
   // ignore: prefer_function_declarations_over_variables, avoid_types_on_closure_parameters
   UiFactory<ProviderProps> Provider = ([Map map]) => (ProviderProps<TValue>(map)..componentFactory = reactDartContext.Provider);
   // ignore: prefer_function_declarations_over_variables, avoid_types_on_closure_parameters

Are you cool just including those fixes in this PR? I think they're pretty low-risk, and are definitely considered bug fixes

Contributor Author

sydneyjodon-wk Jul 28, 2020

Yay!!! Thank you! I'll add that fix

sydneyjodon-wk added 2 commits

July 24, 2020 17:53


          Fix formatting

4283b47


          Fix failing tests

c30ae72

greglittlefield-wf previously requested changes

View reviewed changes

example/hooks/function_components.dart Outdated

Comment on lines 50 to 52

+                FunctionComponentConfig(
+                  displayName: 'UseStateTest',
+                ),

Contributor

greglittlefield-wf Jul 27, 2020

I don't think we want to suggest to consumers to manually create component configs and specify displayNames; could we update these components to have props mixins so that they get generated configs?

That makes me wonder; should we, in the future, generate configs even for function components with UiProps? That way they'd get displayName without having to generate it, and it the syntax would be the same for components that do and don't have their own props classes, making it easier to switch between the two.

Contributor Author

sydneyjodon-wk Jul 28, 2020

That's a good idea! It would make the function component boilerplate a lot more consistent if you only had to create a config when you want to specify your own propsFactory.

example/hooks/function_components.dart Outdated Show resolved Hide resolved

example/hooks/function_components.dart Outdated

Comment on lines 78 to 79

		final ReducerHook<Map, Map, int> state =
		useReducerLazy(reducer, props.initialCount, initializeCount);

Contributor

greglittlefield-wf Jul 27, 2020

#nit could we use RHS typing here?

Also, I feel like "state" might be a bit confusing, but I'm not sure what a better word would be.

Contributor Author

sydneyjodon-wk Jul 28, 2020

Yeah I also don't know what else to name it, but I agree that it's not great

example/hooks/function_components.dart Outdated Show resolved Hide resolved

example/hooks/function_components.dart Outdated Show resolved Hide resolved

example/hooks/function_components.dart Outdated Show resolved Hide resolved

example/hooks/main.dart Outdated Show resolved Hide resolved

example/hooks/function_components.dart Outdated

+              );
+              //final UseContextTest = uiFunction<UiProps>((props) {
+              //  final context = useContext(TestNewContext);

Contributor

greglittlefield-wf Jul 27, 2020

I was able to get to the bottom of this, I think! Looks like there was some missing typing on some of the calls into react-dart's context, unrelated to hooks.

The following changes got it working for me:

diff --git a/lib/src/util/context.dart b/lib/src/util/context.dart
index 40f7e7c5..2299e74f 100644
--- a/lib/src/util/context.dart
+++ b/lib/src/util/context.dart
@@ -49,7 +49,7 @@ class Context<TValue> {
   Context(this.Provider, this.Consumer, this.reactDartContext);
 
   /// The react.dart version of this context.
-  final react.Context reactDartContext;
+  final react.Context<TValue> reactDartContext;
 
   /// The react.js version of this context.
   ReactContext get jsThis => reactDartContext.jsThis;
@@ -237,7 +237,7 @@ class _DO_NOT_USE_OR_YOU_WILL_BE_FIRED {
 ///
 /// Learn more: <https://reactjs.org/docs/context.html#reactcreatecontext>
 Context<TValue> createContext<TValue>([TValue defaultValue, int Function(TValue, TValue) calculateChangedBits]) {
-  react.Context reactDartContext = react.createContext(defaultValue, calculateChangedBits != null ? (dynamic arg1, dynamic arg2) => calculateChangedBits(arg1, arg2) : null);
+  final reactDartContext = react.createContext<TValue>(defaultValue, calculateChangedBits != null ? (dynamic arg1, dynamic arg2) => calculateChangedBits(arg1, arg2) : null);
   // ignore: prefer_function_declarations_over_variables, avoid_types_on_closure_parameters
   UiFactory<ProviderProps> Provider = ([Map map]) => (ProviderProps<TValue>(map)..componentFactory = reactDartContext.Provider);
   // ignore: prefer_function_declarations_over_variables, avoid_types_on_closure_parameters

Are you cool just including those fixes in this PR? I think they're pretty low-risk, and are definitely considered bug fixes

lib/src/component/hooks.dart Outdated

+              /// __Example__:
+              ///
+              /// ```dart
+              /// UiFactory<UiProps> UseStateExample = uiFunction(

Contributor

greglittlefield-wf Jul 27, 2020

Same comnments as in examples around formatting and using generated function configs. I think here we can omit the props mixin declarations themselves, though, and just use them in the typing.


          Merge branch 'function-components-wip' into CPLAT-8665-react-hooks

fa6ce35

sydneyjodon-wk changed the base branch from CPLAT-3873-function-components to function-components-wip

July 28, 2020 16:50

sydneyjodon-wk added 3 commits

July 28, 2020 15:16


          Separate hooks examples into different files

e800d4f


          Update doc comments

f65ebb1


          Fix useContext

6994fd2

sydneyjodon-wk dismissed greglittlefield-wf’s stale review

July 29, 2020 15:38

All feedback addressed

greglittlefield-wf approved these changes

View reviewed changes

Contributor

greglittlefield-wf left a comment

+10

example/hooks/use_layout_effect_example.dart Outdated Show resolved Hide resolved

example/hooks/use_state_example.dart Outdated Show resolved Hide resolved

example/hooks/use_state_example.dart Outdated Show resolved Hide resolved

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels


          Apply suggestions from code review

b2bee79

Address nits

Co-authored-by: Greg Littlefield <[email protected]>

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels

greglittlefield-wf reviewed

View reviewed changes

example/hooks/use_layout_effect_example.dart

+              // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+              // See the License for the specific language governing permissions and
+              // limitations under the License.

Contributor

greglittlefield-wf Jul 29, 2020

my bad, missing an import now 😬

Suggested change



	import 'dart:html';


          Fix import

a168750

greglittlefield-wf reviewed

View reviewed changes

example/hooks/use_layout_effect_example.dart Outdated

+                  final width = useState(0);
+                  final height = useState(0);
+                  final textareaRef = useRef<TextareaElement>();

Contributor

greglittlefield-wf Jul 29, 2020

Suggested change

      
                final textareaRef = useRef<TextareaElement>();
          
                final textareaRef = useRef<TextAreaElement>();

Whoops again 😓

Contributor Author

sydneyjodon-wk Jul 29, 2020

fixed in a168750

greglittlefield-wf approved these changes

View reviewed changes

Contributor

greglittlefield-wf left a comment

+10

greglittlefield-wf merged commit 5ca5761 into function-components-wip

aaronlademann-wf mentioned this pull request

RM-79371 Release over_react 3.8.0 #616

Merged

aaronlademann-wf added the Type: new feature label

sydneyjodon-wk deleted the CPLAT-8665-react-hooks branch

January 28, 2022 16:34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merge Requirements Unmet Type: new feature