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.

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

Fix react-is memo and lazy type checks #17278

Merged
merged 1 commit into from
Nov 5, 2019
Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Nov 5, 2019

I think the react-is package is currently a little broken. This packages typeOf methods are meant to look at React element types (e.g. <MyComponent /> or React.createElement(MyComponent)) not component types (e.g. MyComponent), but it's inconsistent when it comes to lazy and memo types.

This inconsistency can be seen in shallow renderer as well with invariants like this:

isForwardRef(element) || (typeof element.type === 'function' || isMemo(element.type)),

Rather than this:

isForwardRef(element) || (typeof element.type === 'function' || isMemo(element)),

Is this change a bug fix or a breaking change? (Or both?) Let's discuss.

Put another way:

const MemoizedComponent = React.memo(MyComponent);

// MemoizedComponent is a valid element TYPE.
expect(ReactIs.isValidElementType(MemoizedComponent)).toBe(true);

// <MemoizedComponent /> is a valid element.
expect(ReactIs.typeOf(<MemoizedComponent />)).toBe(ReactIs.Memo);
expect(ReactIs.isMemo(<MemoizedComponent />)).toBe(true);

// MemoizedComponent is NOT an element!
// (This used to return a false positive.)
expect(ReactIs.isMemo(MemoizedComponent)).toBe(false);

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 5, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3c2b128:

Sandbox Source
patient-smoke-es4k1 Configuration

@sizebot
Copy link

sizebot commented Nov 5, 2019

Size changes (experimental)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 3c2b128

@sizebot
Copy link

sizebot commented Nov 5, 2019

Size changes (stable)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 3c2b128

@billyjanitsch
Copy link
Contributor

@bvaughn have you seen the discussion in #14546?

Folks from the ecosystem were unsure whether the current behavior was intentional. But it's been around for long enough that several packages now depend on it. So it might be worth calling this a breaking change regardless of the original design intention.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 5, 2019

Thanks for the pointer, @billyjanitsch. I had not seen that discussion. I noticed this inconsistency while looking at a DevTools display complaint.

@@ -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

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

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants