Skip to content

Commit f269074

Browse files
rickhanloniirickhanlonii
authored
Revert "Remove module pattern function component support" (facebook#28670)
This breaks internal tests, so must be something in the refactor. Since it's the top commit let's revert and split into two PRs, one that removes the flag and one that does the refactor, so we can find the bug.
1 parent cc56bed commit f269074

31 files changed

+923
-124
lines changed

‎packages/react-devtools-shared/src/backend/renderer.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ export function getInternalReactConstants(version: string): {
225225
HostSingleton: 27, // Same as above
226226
HostText: 6,
227227
IncompleteClassComponent: 17,
228-
IndeterminateComponent: 2, // removed in 19.0.0
228+
IndeterminateComponent: 2,
229229
LazyComponent: 16,
230230
LegacyHiddenComponent: 23,
231231
MemoComponent: 14,

‎packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ let act;
1414
let React;
1515
let ReactDOM;
1616
let ReactDOMClient;
17+
let PropTypes;
1718
let findDOMNode;
1819

1920
const clone = function (o) {
@@ -98,6 +99,7 @@ describe('ReactComponentLifeCycle', () => {
9899
findDOMNode =
99100
ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.findDOMNode;
100101
ReactDOMClient = require('react-dom/client');
102+
PropTypes = require('prop-types');
101103
});
102104

103105
it('should not reuse an instance when it has been unmounted', async () => {
@@ -1112,6 +1114,72 @@ describe('ReactComponentLifeCycle', () => {
11121114
});
11131115
});
11141116

1117+
if (!require('shared/ReactFeatureFlags').disableModulePatternComponents) {
1118+
// @gate !disableLegacyContext
1119+
it('calls effects on module-pattern component', async () => {
1120+
const log = [];
1121+
1122+
function Parent() {
1123+
return {
1124+
render() {
1125+
expect(typeof this.props).toBe('object');
1126+
log.push('render');
1127+
return <Child />;
1128+
},
1129+
UNSAFE_componentWillMount() {
1130+
log.push('will mount');
1131+
},
1132+
componentDidMount() {
1133+
log.push('did mount');
1134+
},
1135+
componentDidUpdate() {
1136+
log.push('did update');
1137+
},
1138+
getChildContext() {
1139+
return {x: 2};
1140+
},
1141+
};
1142+
}
1143+
Parent.childContextTypes = {
1144+
x: PropTypes.number,
1145+
};
1146+
function Child(props, context) {
1147+
expect(context.x).toBe(2);
1148+
return <div />;
1149+
}
1150+
Child.contextTypes = {
1151+
x: PropTypes.number,
1152+
};
1153+
1154+
const root = ReactDOMClient.createRoot(document.createElement('div'));
1155+
await expect(async () => {
1156+
await act(() => {
1157+
root.render(<Parent ref={c => c && log.push('ref')} />);
1158+
});
1159+
}).toErrorDev(
1160+
'Warning: The <Parent /> component appears to be a function component that returns a class instance. ' +
1161+
'Change Parent to a class that extends React.Component instead. ' +
1162+
"If you can't use a class try assigning the prototype on the function as a workaround. " +
1163+
'`Parent.prototype = React.Component.prototype`. ' +
1164+
"Don't use an arrow function since it cannot be called with `new` by React.",
1165+
);
1166+
await act(() => {
1167+
root.render(<Parent ref={c => c && log.push('ref')} />);
1168+
});
1169+
1170+
expect(log).toEqual([
1171+
'will mount',
1172+
'render',
1173+
'did mount',
1174+
'ref',
1175+
1176+
'render',
1177+
'did update',
1178+
'ref',
1179+
]);
1180+
});
1181+
}
1182+
11151183
it('should warn if getDerivedStateFromProps returns undefined', async () => {
11161184
class MyComponent extends React.Component {
11171185
state = {};

‎packages/react-dom/src/__tests__/ReactCompositeComponent-test.js‎

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -211,27 +211,63 @@ describe('ReactCompositeComponent', () => {
211211
});
212212
});
213213

214-
it('should not support module pattern components', async () => {
215-
function Child({test}) {
216-
return {
217-
render() {
218-
return <div>{test}</div>;
219-
},
220-
};
221-
}
214+
if (require('shared/ReactFeatureFlags').disableModulePatternComponents) {
215+
it('should not support module pattern components', async () => {
216+
function Child({test}) {
217+
return {
218+
render() {
219+
return <div>{test}</div>;
220+
},
221+
};
222+
}
222223

223-
const el = document.createElement('div');
224-
const root = ReactDOMClient.createRoot(el);
225-
await expect(async () => {
226-
await act(() => {
227-
root.render(<Child test="test" />);
228-
});
229-
}).rejects.toThrow(
230-
'Objects are not valid as a React child (found: object with keys {render}).',
231-
);
224+
const el = document.createElement('div');
225+
const root = ReactDOMClient.createRoot(el);
226+
await expect(async () => {
227+
await expect(async () => {
228+
await act(() => {
229+
root.render(<Child test="test" />);
230+
});
231+
}).rejects.toThrow(
232+
'Objects are not valid as a React child (found: object with keys {render}).',
233+
);
234+
}).toErrorDev(
235+
'Warning: The <Child /> component appears to be a function component that returns a class instance. ' +
236+
'Change Child to a class that extends React.Component instead. ' +
237+
"If you can't use a class try assigning the prototype on the function as a workaround. " +
238+
'`Child.prototype = React.Component.prototype`. ' +
239+
"Don't use an arrow function since it cannot be called with `new` by React.",
240+
);
232241

233-
expect(el.textContent).toBe('');
234-
});
242+
expect(el.textContent).toBe('');
243+
});
244+
} else {
245+
it('should support module pattern components', () => {
246+
function Child({test}) {
247+
return {
248+
render() {
249+
return <div>{test}</div>;
250+
},
251+
};
252+
}
253+
254+
const el = document.createElement('div');
255+
const root = ReactDOMClient.createRoot(el);
256+
expect(() => {
257+
ReactDOM.flushSync(() => {
258+
root.render(<Child test="test" />);
259+
});
260+
}).toErrorDev(
261+
'Warning: The <Child /> component appears to be a function component that returns a class instance. ' +
262+
'Change Child to a class that extends React.Component instead. ' +
263+
"If you can't use a class try assigning the prototype on the function as a workaround. " +
264+
'`Child.prototype = React.Component.prototype`. ' +
265+
"Don't use an arrow function since it cannot be called with `new` by React.",
266+
);
267+
268+
expect(el.textContent).toBe('test');
269+
});
270+
}
235271

236272
it('should use default values for undefined props', async () => {
237273
class Component extends React.Component {

‎packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,72 @@ describe('ReactCompositeComponent-state', () => {
527527
]);
528528
});
529529

530+
if (!require('shared/ReactFeatureFlags').disableModulePatternComponents) {
531+
it('should support stateful module pattern components', async () => {
532+
function Child() {
533+
return {
534+
state: {
535+
count: 123,
536+
},
537+
render() {
538+
return <div>{`count:${this.state.count}`}</div>;
539+
},
540+
};
541+
}
542+
543+
const el = document.createElement('div');
544+
const root = ReactDOMClient.createRoot(el);
545+
expect(() => {
546+
ReactDOM.flushSync(() => {
547+
root.render(<Child />);
548+
});
549+
}).toErrorDev(
550+
'Warning: The <Child /> component appears to be a function component that returns a class instance. ' +
551+
'Change Child to a class that extends React.Component instead. ' +
552+
"If you can't use a class try assigning the prototype on the function as a workaround. " +
553+
'`Child.prototype = React.Component.prototype`. ' +
554+
"Don't use an arrow function since it cannot be called with `new` by React.",
555+
);
556+
557+
expect(el.textContent).toBe('count:123');
558+
});
559+
560+
it('should support getDerivedStateFromProps for module pattern components', async () => {
561+
function Child() {
562+
return {
563+
state: {
564+
count: 1,
565+
},
566+
render() {
567+
return <div>{`count:${this.state.count}`}</div>;
568+
},
569+
};
570+
}
571+
Child.getDerivedStateFromProps = (props, prevState) => {
572+
return {
573+
count: prevState.count + props.incrementBy,
574+
};
575+
};
576+
577+
const el = document.createElement('div');
578+
const root = ReactDOMClient.createRoot(el);
579+
await act(() => {
580+
root.render(<Child incrementBy={0} />);
581+
});
582+
583+
expect(el.textContent).toBe('count:1');
584+
await act(() => {
585+
root.render(<Child incrementBy={2} />);
586+
});
587+
expect(el.textContent).toBe('count:3');
588+
589+
await act(() => {
590+
root.render(<Child incrementBy={1} />);
591+
});
592+
expect(el.textContent).toBe('count:4');
593+
});
594+
}
595+
530596
it('should not support setState in componentWillUnmount', async () => {
531597
let subscription;
532598
class A extends React.Component {

‎packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -627,20 +627,33 @@ describe('ReactDOMServerIntegration', () => {
627627
checkFooDiv(await render(<ClassComponent />));
628628
});
629629

630-
itThrowsWhenRendering(
631-
'factory components',
632-
async render => {
630+
if (require('shared/ReactFeatureFlags').disableModulePatternComponents) {
631+
itThrowsWhenRendering(
632+
'factory components',
633+
async render => {
634+
const FactoryComponent = () => {
635+
return {
636+
render: function () {
637+
return <div>foo</div>;
638+
},
639+
};
640+
};
641+
await render(<FactoryComponent />, 1);
642+
},
643+
'Objects are not valid as a React child (found: object with keys {render})',
644+
);
645+
} else {
646+
itRenders('factory components', async render => {
633647
const FactoryComponent = () => {
634648
return {
635649
render: function () {
636650
return <div>foo</div>;
637651
},
638652
};
639653
};
640-
await render(<FactoryComponent />, 1);
641-
},
642-
'Objects are not valid as a React child (found: object with keys {render})',
643-
);
654+
checkFooDiv(await render(<FactoryComponent />, 1));
655+
});
656+
}
644657
});
645658

646659
describe('component hierarchies', function () {

‎packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -879,6 +879,56 @@ describe('ReactErrorBoundaries', () => {
879879
expect(container.firstChild.textContent).toBe('Caught an error: Hello.');
880880
});
881881

882+
// @gate !disableModulePatternComponents
883+
it('renders an error state if module-style context provider throws in componentWillMount', async () => {
884+
function BrokenComponentWillMountWithContext() {
885+
return {
886+
getChildContext() {
887+
return {foo: 42};
888+
},
889+
render() {
890+
return <div>{this.props.children}</div>;
891+
},
892+
UNSAFE_componentWillMount() {
893+
throw new Error('Hello');
894+
},
895+
};
896+
}
897+
BrokenComponentWillMountWithContext.childContextTypes = {
898+
foo: PropTypes.number,
899+
};
900+
901+
const container = document.createElement('div');
902+
const root = ReactDOMClient.createRoot(container);
903+
904+
await expect(async () => {
905+
await act(() => {
906+
root.render(
907+
<ErrorBoundary>
908+
<BrokenComponentWillMountWithContext />
909+
</ErrorBoundary>,
910+
);
911+
});
912+
}).toErrorDev([
913+
'Warning: The <BrokenComponentWillMountWithContext /> component appears to be a function component that ' +
914+
'returns a class instance. ' +
915+
'Change BrokenComponentWillMountWithContext to a class that extends React.Component instead. ' +
916+
"If you can't use a class try assigning the prototype on the function as a workaround. " +
917+
'`BrokenComponentWillMountWithContext.prototype = React.Component.prototype`. ' +
918+
"Don't use an arrow function since it cannot be called with `new` by React.",
919+
...gate(flags =>
920+
flags.disableLegacyContext
921+
? [
922+
'Warning: BrokenComponentWillMountWithContext uses the legacy childContextTypes API which was removed in React 19. Use React.createContext() instead.',
923+
'Warning: BrokenComponentWillMountWithContext uses the legacy childContextTypes API which was removed in React 19. Use React.createContext() instead.',
924+
]
925+
: [],
926+
),
927+
]);
928+
929+
expect(container.firstChild.textContent).toBe('Caught an error: Hello.');
930+
});
931+
882932
it('mounts the error message if mounting fails', async () => {
883933
function renderError(error) {
884934
return <ErrorMessage message={error.message} />;

0 commit comments

Comments
 (0)