Skip to content

Commit b96dbf5

Browse files
aaronlademann-wfaaronlademann-wf
authored
Merge pull request #105 from greglittlefield-wf/getDartComponent-warning
UIP-2629 getDartComponent warning for ReactElements
2 parents df59869 + b299df5 commit b96dbf5

File tree

4 files changed

+84
-11
lines changed

4 files changed

+84
-11
lines changed

‎lib/src/util/dom_util.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ void setSelectionRange(/* TextInputElement | TextAreaElement */Element input, in
128128
if (input is TextAreaElement) {
129129
input.setSelectionRange(start, end, direction);
130130
} else if (input is InputElement && supportsSelectionRange(input)) {
131-
if (browser.isChrome) {
131+
if (browser.isChrome || browser.isFirefox) {
132132
final inputType = input.getAttribute('type');
133133

134134
if (inputType == 'email' || inputType == 'number') {
@@ -161,7 +161,7 @@ int getSelectionStart(Element input) {
161161
} else if (input is TextInputElement && supportsSelectionRange(input)) {
162162
final inputType = input.getAttribute('type');
163163

164-
if (browser.isChrome) {
164+
if (browser.isChrome || browser.isFirefox) {
165165
if (inputType == 'email' || inputType == 'number') {
166166
return null;
167167
}

‎lib/src/util/react_wrappers.dart

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,40 @@ react.Component getDartComponent(/* [1] */ instance) {
280280
return null;
281281
}
282282

283+
assert(() {
284+
if (isValidElement(instance)) {
285+
// `print` instead of `ValidationUtil.warn` so that this message shows up
286+
// in the test output when running `ddev test`.
287+
print(unindent(
288+
'''
289+
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
290+
* WARNING: `getDartComponent`
291+
*
292+
* react-dart 4.0 will no longer support retrieving Dart components from
293+
* `ReactElement` (pre-mounted VDOM instances) in order to prevent memory leaks.
294+
*
295+
* In react-dart 4.0, this call to `getDartComponent` will return `null`.
296+
*
297+
* Please switch over to using the mounted JS component instance.
298+
*
299+
* Example:
300+
* // Before
301+
* var vdom = Button()('Click me');
302+
* react_dom.render(vdom, mountNode);
303+
* var dartInstance = getDartComponent(vdom);
304+
*
305+
* // Fixed
306+
* var vdom = Button()('Click me');
307+
* var jsInstance = react_dom.render(vdom, mountNode);
308+
* var dartInstance = getDartComponent(jsInstance);
309+
*
310+
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
311+
'''
312+
));
313+
}
314+
return true;
315+
});
316+
283317
return _getInternal(instance)?.component;
284318
}
285319

‎test/over_react/util/dom_util_test.dart

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,11 @@ main() {
214214
// See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
215215
test(type, () {
216216
sharedInputSetSelectionRangeTest(type);
217-
}, testOn: 'js && !chrome');
217+
218+
// Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform
219+
// so we can use this to disable certain browser tests during coverage.
220+
// Workaround for https://github.com/Workiva/dart_dev/issues/200
221+
}, testOn: '!(blink || firefox || vm)');
218222
} else {
219223
test(type, () { sharedInputSetSelectionRangeTest(type); });
220224
}
@@ -233,7 +237,7 @@ main() {
233237
});
234238

235239
// See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
236-
group('without throwing an error in Google Chrome when `props.type` is', () {
240+
group('without throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () {
237241
void verifyLackOfException() {
238242
expect(renderedInstance, isNotNull, reason: 'test setup sanity check');
239243
expect(inputElement, isNotNull, reason: 'test setup sanity check');
@@ -266,7 +270,7 @@ main() {
266270
inputElement = findDomNode(renderedInstance);
267271
verifyLackOfException();
268272
});
269-
}, testOn: 'chrome');
273+
}, testOn: 'blink || firefox');
270274
});
271275
});
272276

@@ -301,23 +305,33 @@ main() {
301305
});
302306

303307
group('for an `<input>` of type:', () {
304-
void sharedInputGetSelectionStartTest(String type) {
308+
void sharedInputGetSelectionStartTest(String type, {bool shouldReturnNull: false}) {
305309
renderedInstance = renderAttachedToDocument((Dom.input()
306310
..defaultValue = testValue
307311
..type = type
308312
)());
309313
inputElement = findDomNode(renderedInstance);
314+
setSelectionRange(inputElement, testValue.length, testValue.length);
315+
310316
var selectionStart = getSelectionStart(inputElement);
311317

312-
expect(selectionStart, testValue.length);
318+
expect(selectionStart, shouldReturnNull ? isNull : testValue.length);
313319
}
314320

315321
for (var type in inputTypesWithSelectionRangeSupport) {
316322
if (type == 'email' || type == 'number') {
317323
// See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
324+
test('$type, returning null in Google Chrome (or Dartium/content_shell) or Firefox', () {
325+
sharedInputGetSelectionStartTest(type, shouldReturnNull: true);
326+
}, testOn: 'blink || firefox');
327+
318328
test(type, () {
319-
sharedInputGetSelectionStartTest(type);
320-
}, testOn: 'js && !chrome');
329+
sharedInputGetSelectionStartTest(type, shouldReturnNull: false);
330+
331+
// Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform
332+
// so we can use this to disable certain browser tests during coverage.
333+
// Workaround for https://github.com/Workiva/dart_dev/issues/200
334+
}, testOn: '!(blink || firefox || vm)');
321335
} else {
322336
test(type, () { sharedInputGetSelectionStartTest(type); });
323337
}
@@ -337,7 +351,7 @@ main() {
337351
});
338352

339353
// See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
340-
group('by returning null and not throwing an error in Google Chrome when `props.type` is', () {
354+
group('by returning null and not throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () {
341355
void verifyReturnsNullWithoutException() {
342356
expect(renderedInstance, isNotNull, reason: 'test setup sanity check');
343357
expect(inputElement, isNotNull, reason: 'test setup sanity check');
@@ -366,7 +380,7 @@ main() {
366380
inputElement = findDomNode(renderedInstance);
367381
verifyReturnsNullWithoutException();
368382
});
369-
}, testOn: 'chrome');
383+
}, testOn: 'blink || firefox');
370384
});
371385
});
372386
}

‎test/over_react/util/react_wrappers_test.dart

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
@JS()
1616
library react_wrappers_test;
1717

18+
import 'dart:async';
1819
import 'dart:html';
1920

2021
import 'package:js/js.dart';
@@ -439,6 +440,30 @@ main() {
439440
var renderedInstance = render(Dom.div());
440441
expect(getDartComponent(renderedInstance), isNull);
441442
});
443+
444+
group('', () {
445+
final messageMatcher = contains('react-dart 4.0 will no longer support retrieving Dart components from');
446+
447+
test('warns when passed a ReactElement', () {
448+
ReactElement instance = Wrapper()();
449+
expect(() => getDartComponent(instance), prints(messageMatcher));
450+
}, testOn: 'dart-vm');
451+
452+
test('does not when passed a ReactElement in JS', () {
453+
ReactElement instance = Wrapper()();
454+
expect(() => getDartComponent(instance), isNot(prints(messageMatcher)));
455+
}, testOn: 'js', tags: 'no-ddc');
456+
457+
test('does not warn when passed a ReactComponent', () {
458+
var renderedInstance = render(Wrapper());
459+
expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher)));
460+
});
461+
462+
test('does not warn when passed a DOM component', () {
463+
var renderedInstance = render(Dom.div());
464+
expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher)));
465+
});
466+
});
442467
});
443468

444469
group('getProps', () {

0 commit comments

Comments
 (0)