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

react: Fix test failure from merge conflict. #38603

Merged
merged 1 commit into from
Sep 25, 2019
Merged

react: Fix test failure from merge conflict. #38603

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion types/react/test/index.ts
View file
Open in desktop
Expand Up @@ -402,7 +402,13 @@ const ForwardingRefComponent = React.forwardRef((props: {}, ref: React.Ref<RefCo
return React.createElement(RefComponent, { ref });
});

const ForwardingRefComponentPropTypes: React.WeakValidationMap<Props> = {};
interface AttributeProps extends React.Attributes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
interface AttributeProps extends React.Attributes {
interface AttributeProps {

React.Attributes is not meant to be used that way since it includes props that you never see inside your component or need to validate with propTypes. It's technically only a private type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is broken without extends React.Attributes -- WeakValidationMap<AttributeProps> doesn't have any properties in common with WeakValidationMap<RefAttributes<RefComponent>> from ForwardRefExoticComponent<RefAttributes<RefComponent>>

Does that mean the test is broken or the new propTypes property in ForwardRefExoticComponent is wrong? I suppose I could say

const ForwardingRefComponentPropTypes: React.WeakValidationMap<React.RefAttributes<RefComponent>> = {};

in the test instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know React that well, I'm just trying to get it unblocked for other contributors, so any advice you have is helpful.

Copy link
Collaborator

@eps1lon eps1lon Sep 25, 2019 •
edited

Choose a reason for hiding this comment

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

Well lets merge this if this blocks other work. I'll take look later why we need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I merged it.

@rpokorny Maybe you can take another look too?

hello: string;
world?: string | null;
foo: number;
}

const ForwardingRefComponentPropTypes: React.WeakValidationMap<AttributeProps> = {};
ForwardingRefComponent.propTypes = ForwardingRefComponentPropTypes;

function RefCarryingComponent() {
Expand Down