Enforce no boolean or string constructors #9082

acdlite · 2017-02-28T21:22:22Z

First commit adds a new ESLint rule that errors when Boolean and String constructors are used. Second commit fixes the codebase to adhere to the new rule.

sophiebits · 2017-02-28T21:25:31Z

What's the motivation here? Also we should be wary that String(x) is different from '' + x if x is a Symbol, not sure if that might make a difference.

acdlite · 2017-02-28T21:55:13Z

What's the motivation here?

Slightly better perf in most engines
Consistency
The reason Sebastian gave me is that someone could override the global String or Boolean, which isn't all that compelling to me by itself but combined with the other reasons it is.

acdlite · 2017-02-28T21:56:42Z

Also we should be wary that String(x) is different from '' + x if x is a Symbol

Good point. I'll add a clarification to the error message.

sophiebits

I don't really care. I tend to think that the constructors are a little more readable but I'm not opposed to this if y'all want it.

sebmarkbage · 2017-02-28T23:24:57Z

The reason "someone could override" is interesting is because it makes it harder for VMs and static compilers to assume that it is not overridden.

The String symbol thing is something to vary about, but generally it would be a bad thing to convert a String to a symbol for things like props['' + name] = .... The only time that usage is valid is for logging and then we shouldn't concat put but treat them as separate variables anyway. E.g. warning(false, 'Hello %s', symbol);.

sebmarkbage · 2017-02-28T23:27:29Z

What about Number?

bvaughn

Don't feel strongly about this. Slightly prefer the !!bool or '' + thing syntax anyway.

acdlite · 2017-02-28T23:45:00Z

Oops forget about Number. I'll add that, too.

gaearon · 2017-03-01T02:33:48Z

I prefer Boolean() and String() but can stop caring for the reasons above.

gaearon · 2017-03-01T02:36:24Z

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -167,7 +167,8 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
        callback === null || typeof callback === 'function',
        'render(...): Expected the last optional `callback` argument to be a ' +
        'function. Instead received: %s.',
-        String(callback)
+        // $FlowFixMe - Intentional cast to string


Is there a way to tell Flow it's intentional, or do we plan to leave these here?

I'll ask the Flow folks

Why are we casting this to a string anyway?

It would be better to let the warning printer cast it to a string, so that it can choose how to visualize it. E.g. imagine console.error('warning', obj) in devtools prints this nicer with an option to expand etc.

gaearon · 2017-03-01T02:36:53Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -1034,7 +1034,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
  function hasCapturedError(fiber : Fiber) : boolean {
    // TODO: capturedErrors should store the boundary instance, to avoid needing
    // to check the alternate.
-    return Boolean(
+    return !!(


Isn't this already a boolean?

Yeah, I think it was written that way originally because capturedErrors !== null was just capturedErrors, and when it was null the whole expression would evaluate to null unless it was coerced. I'll change it.

gaearon · 2017-03-01T02:37:03Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -1043,7 +1043,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
  function isFailedBoundary(fiber : Fiber) : boolean {
    // TODO: failedBoundaries should store the boundary instance, to avoid
    // needing to check the alternate.
-    return Boolean(
+    return !!(


Same question.

syranide · 2017-03-01T08:09:47Z

I'm curious, couldn't this be done as a post-process build step instead? (if people prefer the syntax)

gaearon · 2017-03-01T13:07:58Z

Seems easier to enforce than forget and be surprised by output.

sebmarkbage · 2017-03-01T18:58:32Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

@@ -470,7 +470,8 @@ function commitCallbacks(finishedWork : Fiber, queue : UpdateQueue, context : mi
      typeof callback === 'function',
      'Invalid argument passed as callback. Expected a function. Instead ' +
      'received: %s',
-      String(callback)
+      // $FlowFixMe - Intentional cast to string
+      '' + callback


Same here what?

Oh nvm I see your comment above

sebmarkbage · 2017-03-01T18:58:41Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -47,7 +47,8 @@ if (__DEV__) {
      '%s(...): Expected the last optional `callback` argument to be a ' +
      'function. Instead received: %s.',
      callerName,
-      String(callback)
+      // $FlowFixMe - Intentional cast to string
+      '' + callback


sebmarkbage · 2017-03-01T18:59:55Z

src/renderers/shared/fiber/ReactChildFiber.js

@@ -117,7 +117,8 @@ function coerceRef(current: Fiber | null, element: ReactElement) {

 function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) {
  if (returnFiber.type !== 'textarea') {
-    const childrenString = String(newChild);
+    // $FlowFixMe - Intentional cast to string
+    const childrenString = '' + newChild;


Seems like this isn't even used when this is a plain Object.

sebmarkbage · 2017-03-01T19:00:52Z

src/renderers/shared/utils/validateCallback.js

@@ -19,7 +19,8 @@ function validateCallback(callback: ?Function) {
    !callback || typeof callback === 'function',
    'Invalid argument passed as callback. Expected a function. Instead ' +
    'received: %s',
-    String(callback)
+    // $FlowFixMe - Intentional cast to string
+    '' + callback


Shouldn't need to cast to string neither.

sebmarkbage · 2017-03-01T19:01:40Z

It seems to me that all these Flow errors should not be using string coercion anyway. Seems simple enough to fix.

acdlite · 2017-03-01T19:02:14Z

Ok I'll submit a follow up

acdlite · 2017-03-01T19:08:56Z

#9087

justingrant · 2021-08-16T05:46:50Z

FYI, the '' + value pattern is problematic because + calls valueOf and some types' valueOf method will throw. For example, the new ECMAScript "Temporal" date/time API (currently Stage 3) has this throwing-valueOf behavior which prompted #20594 (fixed by #20617). This behavior exists to prevent use of < and > operators to compare Temporal objects.

To avoid similar problems, I just submitted #22064 to change React's use of '' + value to ''.concat(value) and to revise this lint rule's help text. Unfortunately, it looks like ''.concat(value) doesn't pass Flow (facebook/flow#8728).

This PR encouraged this '' + value pattern, so I wanted to ask for your advice. Should React:

Use String(value) ?
Use string interpolation? (which will transpile down to concat(), AFAIK)
Use ''.concat(value) and change the Flow typing of String.prototype.concat to accept mixed args?
Use ''.concat(value) and add type casts to bypass the Flow problem?
Keep '' + value as the general pattern but special-case places where value can be an object?
Something else?

Feel free to answer over in #22064.

cc @ljharb @bvaughn

`''.concat(value)` violates current Flow definitons that require args to concat to be strings only, even though the MDN docs and ES spec allow args to be any type. See facebook/flow#8728. To avoid depending on a change to Flow, this commit changes `''.concat(value)` to `String(value)` and removes the lint rule that prohibits use of String(value). This rule was added in facebook#9082. The other rules in that PR (for Boolean and Number constructors) are not changed in this commit, only the rule for String. If it's decided that using `concat` is better, simply omit this commit and use the previous one instead.

New ESLint rule that warns against Boolean and String constructors

5f92d28

acdlite requested a review from sebmarkbage February 28, 2017 21:22

facebook-github-bot added GH Review: review-needed CLA Signed labels Feb 28, 2017

sebmarkbage requested review from sophiebits, bvaughn, trueadm and gaearon February 28, 2017 23:22

sophiebits approved these changes Feb 28, 2017

View reviewed changes

sebmarkbage approved these changes Feb 28, 2017

View reviewed changes

bvaughn approved these changes Feb 28, 2017

View reviewed changes

gaearon reviewed Mar 1, 2017

View reviewed changes

acdlite added 6 commits February 28, 2017 18:56

Convert usage of String and Boolean constructors

1e9812e

Rename rule to no-primitive-constructors

6ac5fd3

Add caveat about symbols to string error message

b9c96cf

Add error for number constructor, too

2f63b0d

Add test for no-primitive-constructors rule

7d5cc2e

Run test script

b963396

acdlite force-pushed the nobooleanorstringconstructors branch from c05b7f7 to b963396 Compare March 1, 2017 02:57

acdlite merged commit 0585ee8 into facebook:master Mar 1, 2017

sebmarkbage reviewed Mar 1, 2017

View reviewed changes

acdlite mentioned this pull request Mar 1, 2017

Fix string casting and remove Flow suppression comments #9087

Merged

justingrant mentioned this pull request Aug 16, 2021

Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) #22064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce no boolean or string constructors #9082

Enforce no boolean or string constructors #9082

acdlite commented Feb 28, 2017

sophiebits commented Feb 28, 2017

acdlite commented Feb 28, 2017

acdlite commented Feb 28, 2017

sophiebits left a comment

sebmarkbage commented Feb 28, 2017

sebmarkbage commented Feb 28, 2017

bvaughn left a comment

acdlite commented Feb 28, 2017

gaearon commented Mar 1, 2017 •

edited

Loading

gaearon Mar 1, 2017

acdlite Mar 1, 2017

sebmarkbage Mar 1, 2017

gaearon Mar 1, 2017

acdlite Mar 1, 2017 •

edited

Loading

gaearon Mar 1, 2017 •

edited

Loading

syranide commented Mar 1, 2017

gaearon commented Mar 1, 2017

sebmarkbage Mar 1, 2017

acdlite Mar 1, 2017

acdlite Mar 1, 2017

sebmarkbage Mar 1, 2017

sebmarkbage Mar 1, 2017

sebmarkbage Mar 1, 2017

sebmarkbage commented Mar 1, 2017

acdlite commented Mar 1, 2017

acdlite commented Mar 1, 2017

justingrant commented Aug 16, 2021

Enforce no boolean or string constructors #9082

Enforce no boolean or string constructors #9082

Conversation

acdlite commented Feb 28, 2017

sophiebits commented Feb 28, 2017

acdlite commented Feb 28, 2017

acdlite commented Feb 28, 2017

sophiebits left a comment

Choose a reason for hiding this comment

sebmarkbage commented Feb 28, 2017

sebmarkbage commented Feb 28, 2017

bvaughn left a comment

Choose a reason for hiding this comment

acdlite commented Feb 28, 2017

gaearon commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

syranide commented Mar 1, 2017

gaearon commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Mar 1, 2017

acdlite commented Mar 1, 2017

acdlite commented Mar 1, 2017

justingrant commented Aug 16, 2021

gaearon commented Mar 1, 2017 •

edited

Loading

acdlite Mar 1, 2017 •

edited

Loading

gaearon Mar 1, 2017 •

edited

Loading