CPLAT-13185 Fix JsBackedMap for Dart 2.12

[sydneyjodon-wk]

Motivation

Dart 2.12 includes changes to setProperty that disallow passing in Dart functions, since they are not callable by JS code. dart-lang/sdk@d683f33

This breaks passing function props to Dart components, since JsBackedMap stores those values using setProperty under the hood.

  Assertion failed: org-dartlang-sdk:///lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart:166:7
  _isJsObject(f) ||
            !JS<bool>('bool', '# instanceof #.Function', f, global_)
  "Dart function requires `allowInterop` to be passed to JavaScript."
  dart:sdk_internal                                        setProperty
  package:react/react_client/js_backed_map.dart 66:5       _set
  dart:sdk_internal                                        addAll
  package:react/react_client/js_backed_map.dart 96:13      addAll
  package:react/react_client/js_backed_map.dart 34:61      <fn>
  package:react/react_client/js_backed_map.dart 34:73      from
  package:react/src/react_client/factory_util.dart 167:34  generateJsProps
  package:react/react_client/component_factory.dart 86:7   generateExtendedJsProps

This is the reason that react-dart dev builds are currently failing.

We can work around this issue by wrapping Dart functions in an object, which can be passed through without issue.

• This shouldn't affect behavior
  • On the JS side, since JS code should already be treating Dart functions as non-callable, and any functions meant to be called by the JS are properly converted first
  • On the Dart side, since nothing accesses the raw JS values in the first place

Changes

• Added workaround based on Greg's Partial implementaion: 371f5f7
  • Applied the same workaround from JsBackedMap to ContextHelpers
• Tests already cover both allowInterop-wrapped and non-allowInterop-wrapped functions in JsBackedMap and context tests
• Ensure Dart dev channel builds are passing
• Perform consumer testing in over_react, WSD, and major Wdesk repos (DPC, graph_app, home)
  • Since these change affects such a core part of React interop, we want to make sure we're confident it won't cause regressions

QA Steps

• CI passes (including dev channel builds)
• Verify tests adequately cover allowInterop-wrapped and non-allowInterop-wrapped functions (see shared_type_tester.dart)
• Verify that consumer tests are passing in:
  • over_react (except dev channels because of NNBD)
  • web_skin_dart
  • doc_plat_client
  • cerebral-ui
  • graph_app
  • home

[aviary3-wk]

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(1) Security relevant changes were detected

Watched file lib/src/react_client/private_utils.dart modified

Action Items

• Obtain a security review; reviewer should pay special attention to insights listed above
• Verify aviary.yaml coverage of security relevant code

---

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

[semveraudit-wf]

Public API Changes

No changes to the public API found for commit 32b58a7

Showing results for 32b58a7

Powered by semver-audit-service. Please report any problems by filing an issue.
Reported by the dart semver audit client 2.2.0
Browse public API.

Last edited UTC Feb 17 at 19:59:18

[corwinsheahan-wf]

Just one comment!

[corwinsheahan-wf]

Is this functionally any different than _JsBackedMapValue? Could just combine them into a common utility if they're not different

[sydneyjodon-wk]

You're right, I was keeping them separate because I wasn't sure if they should be private. But it probably just makes more sense to add it to the utilities file. Updated in 32b58a7

[aaronlademann-wf]

+1

[evanweible-wf]

+1 security

[aaronlademann-wf]

QA +1

@Workiva/release-management-pp

[rmconsole-wf]

+1 from RM