Skip to content

Commit

Permalink
Upgrade tests to use react/jsx-runtime
Browse files Browse the repository at this point in the history 
Instead of createElement.

We should have done this when we initially released jsx-runtime but better
late than never. The general principle is that our tests should b
 written using the most up-to-date idioms that we recommend for users,
except when explicitly testing an edge case or legacy behavior, like for
backwards compatibility.

Most of the diff is related to tweaking test output and isn't
very interesting.

I did have to workaround an issue related to component stacks. The
component stack logic depends on shared state that lives in the React
module. The problem is that most of our tests reset the React module
state and re-require a fresh instance of React, React DOM, etc.
However, the JSX runtime is not re-required because it's injected by
the compiler as a static import. This means its copy of the shared state
is no longer the same as the one used by React, causing any warning
logged by the JSX runtime to not include a component stack. (This same
issue also breaks string refs, but since we're removing those soon I'm
not so concerned about that.) The solution I went with for now is to
mock the JSX runtime with a proxy that re-requires the module on every
function invocation. I don't love this but it will have to do for now.
What we should really do is migrate our tests away from manually
resetting the module state and use import syntax instead.
  • Loading branch information
@acdlite
acdlite committed Feb 6, 2024
1 parent 4728548 commit 2ad22c4
Show file tree
Hide file tree
Showing 18 changed files with 290 additions and 72 deletions.
1 change: 0 additions & 1 deletion babel.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
module.exports = {
plugins: [
'@babel/plugin-syntax-jsx',
'@babel/plugin-transform-react-jsx',
'@babel/plugin-transform-flow-strip-types',
['@babel/plugin-proposal-class-properties', {loose: true}],
'syntax-trailing-function-commas',
Expand Down
7 changes: 4 additions & 3 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"@babel/plugin-proposal-object-rest-spread": "^7.11.0",
"@babel/plugin-syntax-dynamic-import": "^7.8.3",
"@babel/plugin-syntax-import-meta": "^7.10.4",
"@babel/plugin-syntax-jsx": "^7.10.4",
"@babel/plugin-syntax-jsx": "^7.23.3",
"@babel/plugin-syntax-typescript": "^7.14.5",
"@babel/plugin-transform-arrow-functions": "^7.10.4",
"@babel/plugin-transform-block-scoped-functions": "^7.10.4",
Expand All @@ -27,12 +27,13 @@
"@babel/plugin-transform-modules-commonjs": "^7.10.4",
"@babel/plugin-transform-object-super": "^7.10.4",
"@babel/plugin-transform-parameters": "^7.10.5",
"@babel/plugin-transform-react-jsx-source": "^7.10.5",
"@babel/plugin-transform-react-jsx": "^7.23.4",
"@babel/plugin-transform-react-jsx-development": "^7.22.5",
"@babel/plugin-transform-shorthand-properties": "^7.10.4",
"@babel/plugin-transform-spread": "^7.11.0",
"@babel/plugin-transform-template-literals": "^7.10.5",
"@babel/preset-flow": "^7.10.4",
"@babel/preset-react": "^7.10.4",
"@babel/preset-react": "^7.23.3",
"@babel/traverse": "^7.11.0",
"@rollup/plugin-babel": "^6.0.3",
"@rollup/plugin-commonjs": "^24.0.1",
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ describe('ReactComponent', () => {
root.render(<X />);
});
}).toErrorDev(
'React.createElement: type is invalid -- expected a string (for built-in components) ' +
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.',
),
).rejects.toThrowError(
Expand All @@ -492,7 +492,7 @@ describe('ReactComponent', () => {
root.render(<Y />);
});
}).toErrorDev(
'React.createElement: type is invalid -- expected a string (for built-in components) ' +
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: null.',
),
).rejects.toThrowError(
Expand Down Expand Up @@ -528,7 +528,7 @@ describe('ReactComponent', () => {
root.render(<Foo />);
});
}).toErrorDev(
'React.createElement: type is invalid -- expected a string (for built-in components) ' +
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.',
),
).rejects.toThrowError(
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,7 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
EmptyComponent = <EmptyComponent />;
}).toErrorDev(
'Warning: React.createElement: type is invalid -- expected a string ' +
'Warning: React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: object. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
Expand All @@ -1031,7 +1031,7 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
NullComponent = <NullComponent />;
}).toErrorDev(
'Warning: React.createElement: type is invalid -- expected a string ' +
'Warning: React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.',
{withoutStack: true},
Expand All @@ -1049,7 +1049,7 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
UndefinedComponent = <UndefinedComponent />;
}).toErrorDev(
'Warning: React.createElement: type is invalid -- expected a string ' +
'Warning: React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
Expand Down
10 changes: 8 additions & 2 deletions packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ describe('ReactDeprecationWarnings', () => {
}
class Component extends React.Component {
render() {
return <RefComponent ref="refComponent" __self={this} />;
return React.createElement(RefComponent, {
ref: 'refComponent',
__self: this,
});
}
}
expect(() => {
Expand All @@ -114,7 +117,10 @@ describe('ReactDeprecationWarnings', () => {
}
class Component extends React.Component {
render() {
return <RefComponent ref="refComponent" __self={{}} />;
return React.createElement(RefComponent, {
ref: 'refComponent',
__self: {},
});
}
}

Expand Down
10 changes: 5 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1244,9 +1244,9 @@ describe('ReactIncrementalErrorHandling', () => {
</ErrorBoundary>,
);
await expect(async () => await waitForAll([])).toErrorDev([
'Warning: React.createElement: type is invalid -- expected a string',
'Warning: React.jsx: type is invalid -- expected a string',
// React retries once on error
'Warning: React.createElement: type is invalid -- expected a string',
'Warning: React.jsx: type is invalid -- expected a string',
]);
expect(ReactNoop).toMatchRenderedOutput(
<span
Expand Down Expand Up @@ -1295,9 +1295,9 @@ describe('ReactIncrementalErrorHandling', () => {
</ErrorBoundary>,
);
await expect(async () => await waitForAll([])).toErrorDev([
'Warning: React.createElement: type is invalid -- expected a string',
'Warning: React.jsx: type is invalid -- expected a string',
// React retries once on error
'Warning: React.createElement: type is invalid -- expected a string',
'Warning: React.jsx: type is invalid -- expected a string',
]);
expect(ReactNoop).toMatchRenderedOutput(
<span
Expand All @@ -1317,7 +1317,7 @@ describe('ReactIncrementalErrorHandling', () => {
it('recovers from uncaught reconciler errors', async () => {
const InvalidType = undefined;
expect(() => ReactNoop.render(<InvalidType />)).toErrorDev(
'Warning: React.createElement: type is invalid -- expected a string',
'Warning: React.jsx: type is invalid -- expected a string',
{withoutStack: true},
);
await waitForThrow(
Expand Down
11 changes: 10 additions & 1 deletion packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,22 @@ describe('ReactFreshIntegration', () => {
// eslint-disable-next-line no-new-func
new Function(
'global',
'require',
'React',
'Scheduler',
'exports',
'$RefreshReg$',
'$RefreshSig$',
compiled,
)(global, React, Scheduler, exportsObj, $RefreshReg$, $RefreshSig$);
)(
global,
require,
React,
Scheduler,
exportsObj,
$RefreshReg$,
$RefreshSig$,
);
// Module systems will register exports as a fallback.
// This is useful for cases when e.g. a class is exported,
// and we don't want to propagate the update beyond this module.
Expand Down
3 changes: 1 addition & 2 deletions packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,7 @@ describe('ReactTestRenderer', () => {
expect(() => ReactTestRenderer.create(<Foo />)).toErrorDev(
'Warning: Function components cannot be given refs. Attempts ' +
'to access this ref will fail. ' +
'Did you mean to use React.forwardRef()?\n\n' +
'Check the render method of `Foo`.\n' +
'Did you mean to use React.forwardRef()?\n' +
' in Bar (at **)\n' +
' in Foo (at **)',
);
Expand Down
56 changes: 24 additions & 32 deletions .../react/src/__tests__/ReactElement-test.js → .../src/__tests__/ReactCreateElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ let act;
let React;
let ReactDOMClient;

describe('ReactElement', () => {
// NOTE: This module tests the old, "classic" JSX runtime, React.createElement.
// Do not use JSX syntax in this module; call React.createElement directly.
describe('ReactCreateElement', () => {
let ComponentClass;

beforeEach(() => {
Expand All @@ -24,8 +26,6 @@ describe('ReactElement', () => {

React = require('react');
ReactDOMClient = require('react-dom/client');
// NOTE: We're explicitly not using JSX here. This is intended to test
// classic JS without JSX.
ComponentClass = class extends React.Component {
render() {
return React.createElement('div');
Expand All @@ -48,24 +48,24 @@ describe('ReactElement', () => {
it('should warn when `key` is being accessed on composite element', async () => {
class Child extends React.Component {
render() {
return <div>{this.props.key}</div>;
return React.createElement('div', null, this.props.key);
}
}
class Parent extends React.Component {
render() {
return (
<div>
<Child key="0" />
<Child key="1" />
<Child key="2" />
</div>
return React.createElement(
'div',
null,
React.createElement(Child, {key: '0'}),
React.createElement(Child, {key: '1'}),
React.createElement(Child, {key: '2'}),
);
}
}
const root = ReactDOMClient.createRoot(document.createElement('div'));
await expect(async () => {
await act(() => {
root.render(<Parent />);
root.render(React.createElement(Parent));
});
}).toErrorDev(
'Child: `key` is not a prop. Trying to access it will result ' +
Expand All @@ -76,7 +76,7 @@ describe('ReactElement', () => {
});

it('should warn when `key` is being accessed on a host element', () => {
const element = <div key="3" />;
const element = React.createElement('div', {key: '3'});
expect(() => void element.props.key).toErrorDev(
'div: `key` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
Expand All @@ -89,23 +89,23 @@ describe('ReactElement', () => {
it('should warn when `ref` is being accessed', async () => {
class Child extends React.Component {
render() {
return <div> {this.props.ref} </div>;
return React.createElement('div', null, this.props.ref);
}
}
class Parent extends React.Component {
render() {
return (
<div>
<Child ref={React.createRef()} />
</div>
return React.createElement(
'div',
null,
React.createElement(Child, {ref: React.createRef()}),
);
}
}
const root = ReactDOMClient.createRoot(document.createElement('div'));

await expect(async () => {
await act(() => {
root.render(<Parent />);
root.render(React.createElement(Parent));
});
}).toErrorDev(
'Child: `ref` is not a prop. Trying to access it will result ' +
Expand Down Expand Up @@ -277,8 +277,6 @@ describe('ReactElement', () => {
expect(element.props.children).toEqual([1, 2, 3]);
});

// NOTE: We're explicitly not using JSX here. This is intended to test
// classic JS without JSX.
it('allows static methods to be called using the type property', () => {
class StaticMethodComponentClass extends React.Component {
render() {
Expand All @@ -291,16 +289,12 @@ describe('ReactElement', () => {
expect(element.type.someStaticMethod()).toBe('someReturnValue');
});

// NOTE: We're explicitly not using JSX here. This is intended to test
// classic JS without JSX.
it('is indistinguishable from a plain object', () => {
const element = React.createElement('div', {className: 'foo'});
const object = {};
expect(element.constructor).toBe(object.constructor);
});

// NOTE: We're explicitly not using JSX here. This is intended to test
// classic JS without JSX.
it('should use default prop value when removing a prop', async () => {
class Component extends React.Component {
render() {
Expand All @@ -325,8 +319,6 @@ describe('ReactElement', () => {
expect(instance.props.fruit).toBe('persimmon');
});

// NOTE: We're explicitly not using JSX here. This is intended to test
// classic JS without JSX.
it('should normalize props with default values', async () => {
let instance;
class Component extends React.Component {
Expand Down Expand Up @@ -354,7 +346,7 @@ describe('ReactElement', () => {
it('throws when changing a prop (in dev) after element creation', async () => {
class Outer extends React.Component {
render() {
const el = <div className="moo" />;
const el = React.createElement('div', {className: 'moo'});

if (__DEV__) {
expect(function () {
Expand All @@ -374,7 +366,7 @@ describe('ReactElement', () => {
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(<Outer color="orange" />);
root.render(React.createElement(Outer, {color: 'orange'}));
});
if (__DEV__) {
expect(container.firstChild.className).toBe('moo');
Expand All @@ -387,7 +379,7 @@ describe('ReactElement', () => {
const container = document.createElement('div');
class Outer extends React.Component {
render() {
const el = <div>{this.props.sound}</div>;
const el = React.createElement('div', null, this.props.sound);

if (__DEV__) {
expect(function () {
Expand All @@ -405,7 +397,7 @@ describe('ReactElement', () => {
Outer.defaultProps = {sound: 'meow'};
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<Outer />);
root.render(React.createElement(Outer));
});
expect(container.firstChild.textContent).toBe('meow');
if (__DEV__) {
Expand All @@ -422,12 +414,12 @@ describe('ReactElement', () => {
test = this;
}
render() {
return <div />;
return React.createElement('div');
}
}
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => {
root.render(<Test value={+undefined} />);
root.render(React.createElement(Test, {value: +undefined}));
});
expect(test.props.value).toBeNaN();
});
Expand Down
7 changes: 5 additions & 2 deletions packages/react/src/__tests__/ReactElementValidator-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
'use strict';

// NOTE: We're explicitly not using JSX in this file. This is intended to test
// classic JS without JSX.
// classic React.createElement without JSX.
// TODO: ^ the above note is a bit stale because there are tests in this file
// that do use JSX syntax. We should port them to React.createElement, and also
// confirm there's a corresponding test that uses JSX syntax.

let PropTypes;
let React;
Expand Down Expand Up @@ -548,7 +551,7 @@ describe('ReactElementValidator', () => {
expect(() => {
void (<Foo>{[<div />]}</Foo>);
}).toErrorDev(
'Warning: React.createElement: type is invalid -- expected a string ' +
'Warning: React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
Expand Down
Loading

0 comments on commit 2ad22c4

Please sign in to comment.