Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix react-is memo and lazy type checks #17278

Merged
merged 1 commit into from
Nov 5, 2019
Merged

Fix react-is memo and lazy type checks #17278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-is/src/ReactIs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ export function typeOf(object: any) {
switch ($$typeofType) {
case REACT_CONTEXT_TYPE:
case REACT_FORWARD_REF_TYPE:
case REACT_LAZY_TYPE:
case REACT_MEMO_TYPE:
case REACT_PROVIDER_TYPE:
return $$typeofType;
default:
return $$typeof;
}
}
case REACT_LAZY_TYPE:
case REACT_MEMO_TYPE:
case REACT_PORTAL_TYPE:
return $$typeof;
}
Expand Down
26 changes: 18 additions & 8 deletions packages/react-is/src/__tests__/ReactIs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ describe('ReactIs', () => {

it('should identify context consumers', () => {
const Context = React.createContext(false);
expect(ReactIs.isValidElementType(Context.Consumer)).toBe(true);
expect(ReactIs.typeOf(<Context.Consumer />)).toBe(ReactIs.ContextConsumer);
expect(ReactIs.isContextConsumer(<Context.Consumer />)).toBe(true);
expect(ReactIs.isContextConsumer(<Context.Provider />)).toBe(false);
Expand All @@ -79,6 +80,7 @@ describe('ReactIs', () => {

it('should identify context providers', () => {
const Context = React.createContext(false);
expect(ReactIs.isValidElementType(Context.Provider)).toBe(true);
expect(ReactIs.typeOf(<Context.Provider />)).toBe(ReactIs.ContextProvider);
expect(ReactIs.isContextProvider(<Context.Provider />)).toBe(true);
expect(ReactIs.isContextProvider(<Context.Consumer />)).toBe(false);
Expand Down Expand Up @@ -106,13 +108,15 @@ describe('ReactIs', () => {

it('should identify ref forwarding component', () => {
const RefForwardingComponent = React.forwardRef((props, ref) => null);
expect(ReactIs.isValidElementType(RefForwardingComponent)).toBe(true);
expect(ReactIs.typeOf(<RefForwardingComponent />)).toBe(ReactIs.ForwardRef);
Copy link
Contributor

@wsmd wsmd Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed an inconsistency here with how typeOf works when we pass a momozied element vs when we pass a ref-forwarding element. More info here #17274 (comment)

I think this PR addresses this inconsistency between the two types, but I'm still curious, what's the intended behavior for typeOf(RefForwardingComponent) and typeOf(<RefForwardingComponent />) – should we expect to get back ReactIs.ForwardRef in both cases? Same question goes for memoized elements/components.

For example:

expect(ReactIs.typeOf(<Memoized />)).toBe(ReactIs.Memo);
expect(ReactIs.typeOf(Memoized)).toBe(ReactIs.Memo);

expect(ReactIs.typeOf(<RefForwardingComponent />)).toBe(ReactIs.ForwardRef);
expect(ReactIs.typeOf(RefForwardingComponent)).toBe(ReactIs.ForwardRef);

The proposed bug fix in #17274 relies on those assertions above being valid.

Copy link
Contributor Author

@bvaughn bvaughn Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR addresses this inconsistency between the two types, but I'm still curious, what's the intended behavior for typeOf(RefForwardingComponent) and typeOf() – should we expect to get back ReactIs.ForwardRef in both cases?

No. That's the primary purpose of this PR, to not support that use case. The typeOf methods are meant to look at React element types (e.g. <MyComponent /> or React.createElement(MyComponent)) not component types (e.g. MyComponent)

I've seen #17274. I have some draft comments on it but got distracted by this PR first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification @bvaughn! Exactly the answer I was looking for!

expect(ReactIs.isForwardRef(<RefForwardingComponent />)).toBe(true);
expect(ReactIs.isForwardRef({type: ReactIs.StrictMode})).toBe(false);
expect(ReactIs.isForwardRef(<div />)).toBe(false);
});

it('should identify fragments', () => {
expect(ReactIs.isValidElementType(React.Fragment)).toBe(true);
expect(ReactIs.typeOf(<React.Fragment />)).toBe(ReactIs.Fragment);
expect(ReactIs.isFragment(<React.Fragment />)).toBe(true);
expect(ReactIs.isFragment({type: ReactIs.Fragment})).toBe(false);
Expand All @@ -124,35 +128,40 @@ describe('ReactIs', () => {
it('should identify portals', () => {
const div = document.createElement('div');
const portal = ReactDOM.createPortal(<div />, div);
expect(ReactIs.isValidElementType(portal)).toBe(false);
expect(ReactIs.typeOf(portal)).toBe(ReactIs.Portal);
expect(ReactIs.isPortal(portal)).toBe(true);
expect(ReactIs.isPortal(div)).toBe(false);
});

it('should identify memo', () => {
const Component = () => React.createElement('div');
const memoized = React.memo(Component);
expect(ReactIs.typeOf(memoized)).toBe(ReactIs.Memo);
expect(ReactIs.isMemo(memoized)).toBe(true);
expect(ReactIs.isMemo(Component)).toBe(false);
const Memoized = React.memo(Component);
expect(ReactIs.isValidElementType(Memoized)).toBe(true);
expect(ReactIs.typeOf(<Memoized />)).toBe(ReactIs.Memo);
expect(ReactIs.isMemo(<Memoized />)).toBe(true);
expect(ReactIs.isMemo(<Component />)).toBe(false);
});

it('should identify lazy', () => {
const Component = () => React.createElement('div');
const lazyComponent = React.lazy(() => Component);
expect(ReactIs.typeOf(lazyComponent)).toBe(ReactIs.Lazy);
expect(ReactIs.isLazy(lazyComponent)).toBe(true);
expect(ReactIs.isLazy(Component)).toBe(false);
const LazyComponent = React.lazy(() => Component);
expect(ReactIs.isValidElementType(LazyComponent)).toBe(true);
expect(ReactIs.typeOf(<LazyComponent />)).toBe(ReactIs.Lazy);
expect(ReactIs.isLazy(<LazyComponent />)).toBe(true);
expect(ReactIs.isLazy(<Component />)).toBe(false);
});

it('should identify strict mode', () => {
expect(ReactIs.isValidElementType(React.StrictMode)).toBe(true);
expect(ReactIs.typeOf(<React.StrictMode />)).toBe(ReactIs.StrictMode);
expect(ReactIs.isStrictMode(<React.StrictMode />)).toBe(true);
expect(ReactIs.isStrictMode({type: ReactIs.StrictMode})).toBe(false);
expect(ReactIs.isStrictMode(<div />)).toBe(false);
});

it('should identify suspense', () => {
expect(ReactIs.isValidElementType(React.Suspense)).toBe(true);
expect(ReactIs.typeOf(<React.Suspense />)).toBe(ReactIs.Suspense);
expect(ReactIs.isSuspense(<React.Suspense />)).toBe(true);
expect(ReactIs.isSuspense({type: ReactIs.Suspense})).toBe(false);
Expand All @@ -161,6 +170,7 @@ describe('ReactIs', () => {
});

it('should identify profile root', () => {
expect(ReactIs.isValidElementType(React.Profiler)).toBe(true);
expect(
ReactIs.typeOf(<React.Profiler id="foo" onRender={jest.fn()} />),
).toBe(ReactIs.Profiler);
Expand Down
10 changes: 5 additions & 5 deletions packages/react-test-renderer/src/ReactShallowRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ class ReactShallowRenderer {
);
invariant(
isForwardRef(element) ||
(typeof element.type === 'function' || isMemo(element.type)),
(typeof element.type === 'function' || isMemo(element)),
'ReactShallowRenderer render(): Shallow rendering works only with custom ' +
'components, but the provided element type was `%s`.',
Array.isArray(element.type)
Expand All @@ -559,15 +559,15 @@ class ReactShallowRenderer {
this._reset();
}

const elementType = isMemo(element.type) ? element.type.type : element.type;
const elementType = isMemo(element) ? element.type.type : element.type;
const previousElement = this._element;

this._rendering = true;
this._element = element;
this._context = getMaskedContext(elementType.contextTypes, context);

// Inner memo component props aren't currently validated in createElement.
if (isMemo(element.type) && elementType.propTypes) {
if (isMemo(element) && elementType.propTypes) {
currentlyValidatingElement = element;
checkPropTypes(
elementType.propTypes,
Expand Down Expand Up @@ -618,7 +618,7 @@ class ReactShallowRenderer {
this._mountClassComponent(elementType, element, this._context);
} else {
let shouldRender = true;
if (isMemo(element.type) && previousElement !== null) {
if (isMemo(element) && previousElement !== null) {
// This is a Memo component that is being re-rendered.
const compare = element.type.compare || shallowEqual;
if (compare(previousElement.props, element.props)) {
Expand Down Expand Up @@ -807,7 +807,7 @@ function getDisplayName(element) {
} else if (typeof element.type === 'string') {
return element.type;
} else {
const elementType = isMemo(element.type) ? element.type.type : element.type;
const elementType = isMemo(element) ? element.type.type : element.type;
return elementType.displayName || elementType.name || 'Unknown';
}
}
Expand Down