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

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

Merged
merged 5 commits into from
Sep 10, 2020

Conversation

greglittlefield-wf
Copy link
Contributor

Motivation

Workiva/react-dart#275 fixes forwardRef/memo bugs and exposes new versions of the APIs, and we need to consume them in over_react.

Changes

  • Update uiForwardRef to use forwardRef2 under the hood
  • Update memo to use memo2 under the hood

Release Notes

Fix uiForwardRef and memo being passed JSified versions of props.

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

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

@aviary3-wk
Copy link

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.

@rm-astro-wf rm-astro-wf changed the title Consume/update typing for forwardRef2/memo2 functions CPLAT-11977 Consume/update typing for forwardRef2/memo2 functions Aug 20, 2020
@@ -31,21 +30,12 @@ class FancyInputApi {
FancyInputApi(this.focus);
}

UiFactory<FancyInputProps> FancyInput = uiForwardRef((props, ref) {
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 simplified this while i was in here fixing the typing on forwardedRef

@@ -470,7 +470,7 @@ void useLayoutEffect(dynamic Function() sideEffect, [List<Object> dependencies])
/// ```
///
/// Learn more: <https://reactjs.org/docs/hooks-reference.html#useimperativehandle>.
void useImperativeHandle(Ref ref, dynamic Function() createHandle, [List<dynamic> dependencies]) =>
void useImperativeHandle(dynamic ref, dynamic Function() createHandle, [List<dynamic> dependencies]) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has just been updated to match the typing of the react-dart version

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), 'Anonymous');
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. forwardRef2 uses name, not displayName
  2. An empty string is used instead of Anonymous in forwardRef2 (the empty string shows up as "Anonymous" in the dev tools, so functionality is equivalent), so these needed to be updated

factory = react_interop.forwardRef(_uiFunctionWrapper);
}
// Always pass displayName, even if it's empty,
// since we don't want forwardRef2 to use _uiFunctionWrapper as the name.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under the hood forwardRef2 falls back to the name of the function if displayName is null, as opposed to the old default of Anonymous.

If the name is empty, it will still show up as "Anonymous" in the dev tools, so functionality is equivalent.

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@sydneyjodon-wk
Copy link
Contributor

QA+1

  • smoke test examples

@greglittlefield-wf
Copy link
Contributor Author

This is ready for review/QA refresh and merge! @aaronlademann-wf @sydneyjodon-wk

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@sydneyjodon-wk
Copy link
Contributor

QA+1

@greglittlefield-wf
Copy link
Contributor Author

@Workiva/release-management-p

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@Workiva/release-management-pp

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole2-wf rmconsole2-wf merged commit 3e9bb81 into master Sep 10, 2020
@rmconsole2-wf rmconsole2-wf deleted the forwardRef-memo-fixes branch September 10, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants