Skip to content

Commit

Permalink
Use String(value) for coercion in non-prod files
Browse files Browse the repository at this point in the history
This commit switches non-production code from `'' + value` (which
throws for Temporal objects and symbols) to instead use `String(value)`
which won't throw for these or other future plus-phobic types.

"Non-produciton code" includes anything not bundled into user apps:
* Tests and test utilities. Note that I didn't change legacy React
  test fixtures because I assumed it was good for those files to
  act just like old React, including coercion behavior.
* Build scripts
* Dev tools package - In addition to switching to `String`, I also
  removed special-case code for coercing symbols which is now
  unnecessary.
  • Loading branch information
justingrant committed Sep 27, 2021
1 parent 64a72a4 commit de90529
Show file tree
Hide file tree
Showing 23 changed files with 47 additions and 61 deletions.
4 changes: 2 additions & 2 deletions dangerfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ function row(result) {
let headSha;
let baseSha;
try {
headSha = (readFileSync(HEAD_DIR + '/COMMIT_SHA') + '').trim();
baseSha = (readFileSync(BASE_DIR + '/COMMIT_SHA') + '').trim();
headSha = String(readFileSync(HEAD_DIR + '/COMMIT_SHA')).trim();
baseSha = String(readFileSync(BASE_DIR + '/COMMIT_SHA')).trim();
} catch {
warn(
"Failed to read build artifacts. It's possible a build configuration " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class ErrorBoundary extends React.Component {
if (this.state.error) {
return <p>Captured an error: {this.state.error.message}</p>;
} else {
return <p>Captured an error: {'' + this.state.error}</p>;
return <p>Captured an error: {String(this.state.error)}</p>;
}
}
if (this.state.shouldThrow) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ describe('ReactHooksInspectionIntegration', () => {
expect(tree[0].id).toEqual(0);
expect(tree[0].isStateEditable).toEqual(false);
expect(tree[0].name).toEqual('OpaqueIdentifier');
expect((tree[0].value + '').startsWith('c_')).toBe(true);
expect(String(tree[0].value).startsWith('c_')).toBe(true);

expect(tree[1]).toEqual({
id: 1,
Expand Down Expand Up @@ -646,7 +646,7 @@ describe('ReactHooksInspectionIntegration', () => {
expect(tree[0].id).toEqual(0);
expect(tree[0].isStateEditable).toEqual(false);
expect(tree[0].name).toEqual('OpaqueIdentifier');
expect((tree[0].value + '').startsWith('c_')).toBe(true);
expect(String(tree[0].value).startsWith('c_')).toBe(true);

expect(tree[1]).toEqual({
id: 1,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-extensions/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function createPanelIfReactLoaded() {

function initBridgeAndStore() {
const port = chrome.runtime.connect({
name: '' + tabId,
name: String(tabId),
});
// Looks like `port.onDisconnect` does not trigger on in-tab navigation like new URL or back/forward navigation,
// so it makes no sense to handle it here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function getData(internalInstance: InternalInstance) {
// != used deliberately here to catch undefined and null
if (internalInstance._currentElement != null) {
if (internalInstance._currentElement.key) {
key = '' + internalInstance._currentElement.key;
key = String(internalInstance._currentElement.key);
}

const elementType = internalInstance._currentElement.type;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1847,7 +1847,7 @@ export function attach(

// This check is a guard to handle a React element that has been modified
// in such a way as to bypass the default stringification of the "key" property.
const keyString = key === null ? null : '' + key;
const keyString = key === null ? null : String(key);
const keyStringID = getStringID(keyString);

pushOperation(TREE_OPERATION_ADD);
Expand Down
13 changes: 3 additions & 10 deletions packages/react-devtools-shared/src/backend/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,7 @@ export function format(
): string {
const args = inputArgs.slice();

// Symbols cannot be concatenated with Strings.
let formatted: string =
typeof maybeMessage === 'symbol'
? maybeMessage.toString()
: '' + maybeMessage;
let formatted: string = String(maybeMessage);

// If the first argument is a string, check for substitutions.
if (typeof maybeMessage === 'string') {
Expand Down Expand Up @@ -203,17 +199,14 @@ export function format(
// Arguments that remain after formatting.
if (args.length) {
for (let i = 0; i < args.length; i++) {
const arg = args[i];

// Symbols cannot be concatenated with Strings.
formatted += ' ' + (typeof arg === 'symbol' ? arg.toString() : arg);
formatted += ' ' + String(args[i]);
}
}

// Update escaped %% values.
formatted = formatted.replace(/%{2,2}/g, '%');

return '' + formatted;
return String(formatted);
}

export function isSynchronousXHRSupported(): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export default class ErrorBoundary extends Component<Props, State> {
error !== null &&
error.hasOwnProperty('message')
? error.message
: '' + error;
: String(error);

const callStack =
typeof error === 'object' &&
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/devtools/views/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ export function alphaSortEntries(
): number {
const a = entryA[0];
const b = entryB[0];
if ('' + +a === a) {
if ('' + +b !== b) {
if (String(+a) === a) {
if (String(+b) !== b) {
return -1;
}
return +a < +b ? -1 : 1;
Expand Down
12 changes: 3 additions & 9 deletions packages/react-devtools-shared/src/hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,7 @@ export function installHook(target: any): DevToolsHook | null {
const args = inputArgs.slice();

// Symbols cannot be concatenated with Strings.
let formatted: string =
typeof maybeMessage === 'symbol'
? maybeMessage.toString()
: '' + maybeMessage;
let formatted = String(maybeMessage);

// If the first argument is a string, check for substitutions.
if (typeof maybeMessage === 'string') {
Expand Down Expand Up @@ -216,17 +213,14 @@ export function installHook(target: any): DevToolsHook | null {
// Arguments that remain after formatting.
if (args.length) {
for (let i = 0; i < args.length; i++) {
const arg = args[i];

// Symbols cannot be concatenated with Strings.
formatted += ' ' + (typeof arg === 'symbol' ? arg.toString() : arg);
formatted += ' ' + String(args[i]);
}
}

// Update escaped %% values.
formatted = formatted.replace(/%{2,2}/g, '%');

return '' + formatted;
return String(formatted);
}

let unpatchFn = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ function normalizeSourcePath(
const {sourceRoot} = map;
let source = sourceInput;

// eslint-disable-next-line react-internal/no-primitive-constructors
source = String(source);
// Some source maps produce relative source paths like "./foo.js" instead of
// "foo.js". Normalize these first so that future comparisons will succeed.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ export function formatDataForPreview(
return data;
default:
try {
return truncateForDisplay('' + data);
return truncateForDisplay(String(data));
} catch (error) {
return 'unserializable';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1745,7 +1745,7 @@ describe('ReactDOMServerHooks', () => {

it('useOpaqueIdentifier warns when there is a hydration error and we are using ID as a string', async () => {
function Child({appId}) {
return <div aria-labelledby={appId + ''} />;
return <div aria-labelledby={String(appId)} />;
}
function App() {
const id = useOpaqueIdentifier();
Expand All @@ -1769,7 +1769,7 @@ describe('ReactDOMServerHooks', () => {

it('useOpaqueIdentifier warns when there is a hydration error and we are using ID as a string', async () => {
function Child({appId}) {
return <div aria-labelledby={appId + ''} />;
return <div aria-labelledby={String(appId)} />;
}
function App() {
const id = useOpaqueIdentifier();
Expand All @@ -1793,7 +1793,7 @@ describe('ReactDOMServerHooks', () => {

it('useOpaqueIdentifier warns if you try to use the result as a string in a child component', async () => {
function Child({appId}) {
return <div aria-labelledby={appId + ''} />;
return <div aria-labelledby={String(appId)} />;
}
function App() {
const id = useOpaqueIdentifier();
Expand All @@ -1817,7 +1817,7 @@ describe('ReactDOMServerHooks', () => {
it('useOpaqueIdentifier warns if you try to use the result as a string', async () => {
function App() {
const id = useOpaqueIdentifier();
return <div aria-labelledby={id + ''} />;
return <div aria-labelledby={String(id)} />;
}

const container = document.createElement('div');
Expand All @@ -1836,7 +1836,7 @@ describe('ReactDOMServerHooks', () => {

it('useOpaqueIdentifier warns if you try to use the result as a string in a child component wrapped in a Suspense', async () => {
function Child({appId}) {
return <div aria-labelledby={appId + ''} />;
return <div aria-labelledby={String(appId)} />;
}
function App() {
const id = useOpaqueIdentifier();
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/__tests__/ReactMultiChildText-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const expectChildren = function(container, children) {
} else {
expect(textNode != null).toBe(true);
expect(textNode.nodeType).toBe(3);
expect(textNode.data).toBe('' + children);
expect(textNode.data).toBe(String(children));
}
} else {
let mountIndex = 0;
Expand All @@ -55,7 +55,7 @@ const expectChildren = function(container, children) {
if (typeof child === 'string') {
textNode = outerNode.childNodes[mountIndex];
expect(textNode.nodeType).toBe(3);
expect(textNode.data).toBe('' + child);
expect(textNode.data).toBe(child);
mountIndex++;
} else {
const elementDOMNode = outerNode.childNodes[mountIndex];
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/test-utils/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ function validateClassInstance(inst, methodName) {
return;
}
let received;
const stringified = '' + inst;
const stringified = String(inst);
if (isArray(inst)) {
received = 'an array';
} else if (inst && inst.nodeType === ELEMENT_NODE && inst.tagName) {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/__tests__/ReactCache-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ describe('ReactCache', () => {
function Unrelated() {
const [count, _updateUnrelated] = useState(0);
updateUnrelated = _updateUnrelated;
return <Text text={count + ''} />;
return <Text text={String(count)} />;
}

const root = ReactNoop.createRoot();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2754,7 +2754,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create insertion [current: ${committedText}]`,
);
committedText = props.count + '';
committedText = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy insertion [current: ${committedText}]`,
Expand Down Expand Up @@ -2817,7 +2817,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create insertion [current: ${committedText}]`,
);
committedText = props.count + '';
committedText = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy insertion [current: ${committedText}]`,
Expand All @@ -2828,7 +2828,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create layout [current: ${committedText}]`,
);
committedText = props.count + '';
committedText = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy layout [current: ${committedText}]`,
Expand Down Expand Up @@ -2886,7 +2886,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create Insertion 1 for Component A [A: ${committedA}, B: ${committedB}]`,
);
committedA = props.count + '';
committedA = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy Insertion 1 for Component A [A: ${committedA}, B: ${committedB}]`,
Expand All @@ -2897,7 +2897,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create Insertion 2 for Component A [A: ${committedA}, B: ${committedB}]`,
);
committedA = props.count + '';
committedA = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy Insertion 2 for Component A [A: ${committedA}, B: ${committedB}]`,
Expand Down Expand Up @@ -2934,7 +2934,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create Insertion 1 for Component B [A: ${committedA}, B: ${committedB}]`,
);
committedB = props.count + '';
committedB = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy Insertion 1 for Component B [A: ${committedA}, B: ${committedB}]`,
Expand All @@ -2945,7 +2945,7 @@ describe('ReactHooksWithNoopRenderer', () => {
Scheduler.unstable_yieldValue(
`Create Insertion 2 for Component B [A: ${committedA}, B: ${committedB}]`,
);
committedB = props.count + '';
committedB = String(props.count);
return () => {
Scheduler.unstable_yieldValue(
`Destroy Insertion 2 for Component B [A: ${committedA}, B: ${committedB}]`,
Expand Down Expand Up @@ -3140,7 +3140,7 @@ describe('ReactHooksWithNoopRenderer', () => {
useLayoutEffect(() => {
// Normally this would go in a mutation effect, but this test
// intentionally omits a mutation effect.
committedText = props.count + '';
committedText = String(props.count);

Scheduler.unstable_yieldValue(
`Mount layout [current: ${committedText}]`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1110,7 +1110,7 @@ describe('ReactIncremental', () => {
}
render() {
Scheduler.unstable_yieldValue('Bar:' + this.props.x);
return <span prop={'' + (this.props.x === this.state.y)} />;
return <span prop={String(this.props.x === this.state.y)} />;
}
}

Expand Down Expand Up @@ -1159,7 +1159,7 @@ describe('ReactIncremental', () => {
Scheduler.unstable_yieldValue(
'Bar:' + this.props.x + '-' + this.props.step,
);
return <span prop={'' + (this.props.x === this.state.y)} />;
return <span prop={String(this.props.x === this.state.y)} />;
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/__tests__/ReactMemo-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ describe('memo', () => {
class CounterInner extends React.Component {
static defaultProps = {suffix: '!'};
render() {
return <Text text={this.props.count + '' + this.props.suffix} />;
return <Text text={this.props.count + String(this.props.suffix)} />;
}
}
const Counter = memo(CounterInner);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('ReactPersistent', () => {
function createPortal(children, containerInfo, implementation, key) {
return {
$$typeof: Symbol.for('react.portal'),
key: key == null ? null : '' + key,
key: key == null ? null : String(key),
children,
containerInfo,
implementation,
Expand Down
4 changes: 2 additions & 2 deletions scripts/flow/runFlow.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ async function runFlow(renderer, args) {
const srcStat = fs.statSync(__dirname + '/config/flowconfig');
const destPath = './.flowconfig';
if (fs.existsSync(destPath)) {
const oldConfig = fs.readFileSync(destPath) + '';
const newConfig = fs.readFileSync(srcPath) + '';
const oldConfig = String(fs.readFileSync(destPath));
const newConfig = String(fs.readFileSync(srcPath));
if (oldConfig !== newConfig) {
// Use the mtime to detect if the file was manually edited. If so,
// log an error.
Expand Down
4 changes: 2 additions & 2 deletions scripts/merge-fork/merge-fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ for (const {base: baseFilename, from, to} of getTransforms()) {
}
);
if (gitShowResult.status !== 0) {
console.error('' + gitShowResult.stderr);
console.error(String(gitShowResult.stderr));
continue;
}

Expand All @@ -104,6 +104,6 @@ for (const {base: baseFilename, from, to} of getTransforms()) {
});

if (mergeFileResult.status !== 0) {
console.error('' + mergeFileResult.stderr);
console.error(String(mergeFileResult.stderr));
}
}
Loading

0 comments on commit de90529

Please sign in to comment.