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

Fix JsBackedMap sometimes returning undefined instead of null #396

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/react_client/js_backed_map.dart
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ class JsBackedMap extends MapBase<dynamic, dynamic> {

@override
dynamic /*?*/ operator [](Object? key) {
return DartValueWrapper.unwrapIfNeeded(js_util.getProperty(jsObject, nonNullableJsPropertyName(key)));
// `?? null` as a workaround to `undefined` coming through and causing issues: https://github.com/dart-lang/sdk/issues/36116
// ignore: unnecessary_null_in_if_null_operators
return DartValueWrapper.unwrapIfNeeded(js_util.getProperty(jsObject, nonNullableJsPropertyName(key)) ?? null);
}

@override
Expand Down
53 changes: 53 additions & 0 deletions test/react_client/js_backed_map_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ library react.js_backed_map_test.dart;
import 'package:js/js.dart';
import 'package:js/js_util.dart';
import 'package:react/react_client/js_backed_map.dart';
import 'package:react/react_client/react_interop.dart';
import 'package:test/test.dart';

import '../shared_type_tester.dart';
Expand Down Expand Up @@ -49,6 +50,40 @@ main() {
});
});

// Tests related to our workaround for https://github.com/dart-lang/sdk/issues/36116.
group(
'map lookup returns null instead of undefined, so that the value does not interfere with Dart function calls:',
() {
test('test setup check: getNullishValueType works as expected', () {
expect(getNullishValueType(jsUndefined), 'undefined');
expect(getNullishValueType(jsNull), 'null');
});

test('returns null and not undefined', () {
final jsBackedMap = JsBackedMap();
final value = jsBackedMap['missingKey'];
// We can't use `identical(value, jsUndefined)` (or the `same` matcher which uses `identical`)
// since it's special-cased to return true when passed two nullish (null or undefined) values.
// Instead, we'll use a custom JS interop function to classify the value
expect(getNullishValueType(value), 'null');
});

test('test setup check: triggersDdcOptionalArgumentBug works as expected to reproduce and detect the bug', () {
expect(triggersDdcOptionalArgumentBug(jsUndefined), isTrue,
reason: 'if this expectation fails in newer Dart SDKs, then this bug may have been fixed.');
expect(triggersDdcOptionalArgumentBug(null), isFalse);
// Since this bug only affects DDC, this test will fail in dart2js.
}, tags: 'no-dart2js');

// We only need to run this test in DDC since the bug does not effect dart2js, but it doesn't hurt to also
// validate behavior in dart2js, and could catch new, similar dart2js bugs.
test('regression test: returned value does not trigger argument issues', () {
final jsBackedMap = JsBackedMap();
final value = jsBackedMap['missingKey'];
expect(triggersDdcOptionalArgumentBug(value), isFalse);
});
});

group('constructor', () {
test('(default) creates an instance backed by a new JS object', () {
final jsBackedMap = JsBackedMap();
Expand Down Expand Up @@ -210,5 +245,23 @@ main() {

class TestObject {}

@JS()
external String getNullishValueType(dynamic value);

class _Default {
const _Default();
}

const _default = _Default();

bool _defaultArgumentSpecifiedUsed([x = _default]) => x == _default;

bool triggersDdcOptionalArgumentBug(dynamic value) {
// We'd expect this call to always return false,
// but it returns true in DDC if `value` is `undefined`.
// https://github.com/dart-lang/sdk/issues/36116
return _defaultArgumentSpecifiedUsed(value);
}

@JS('Object.freeze')
external void _objectFreeze(JsMap object);
10 changes: 10 additions & 0 deletions test/react_client/js_backed_map_test.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@

<script>
function JsType() {}

function getNullishValueType(value) {
if (typeof value === 'undefined') {
return 'undefined';
}
if (value === null) {
return 'null';
}
throw new Error('value was neither null nor undefined');
}
</script>

<link rel="x-dart-test" href="js_backed_map_test.dart">
Expand Down
4 changes: 4 additions & 0 deletions test/shared_type_tester.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ void sharedTypeTests(
bool skipJsAnonInteropTypes = false,
bool skipJsInteropTypes = true,
}) {
test('null', () {
testTypeValue(null);
});

if (!skipNormalDartObjects) {
test('normal Dart objects', () {
final object = Foo('f');
Expand Down
Loading