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] Support Flow type spread #2446

Merged
merged 1 commit into from Oct 16, 2019

Conversation

@moroine
Copy link
Contributor

@moroine moroine commented Oct 5, 2019

This PR aims to support type spread like:

// @flow
import * as React from 'react';

type DefaultProps = {|
  foo: number,
|}

type Props = {
  bar: string,
};

function MyComponent(props: Props) {}

MyComponent.defaultProps = {
  foo: 42,
};

This PR also fixes #2138

@ljharb ljharb added the flow label Oct 8, 2019
@moroine moroine requested a review from ljharb Oct 9, 2019
@moroine
Copy link
Contributor Author

@moroine moroine commented Oct 9, 2019

What's your opinion about trying to resolve types from external files, or add an option to warn when type can't be resolved

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Oct 12, 2019

@moroine i'm not a flow user, so i'm not sure. what do you think?

@ljharb
ljharb approved these changes Oct 12, 2019
@moroine
Copy link
Contributor Author

@moroine moroine commented Oct 12, 2019

@ljharb I think it can be a good idea to warn first, as I discover that limitation only when reading the code.

@ljharb ljharb force-pushed the moroine:feature/support-type-spread branch from f72490c to 11dc56b Oct 16, 2019
@ljharb ljharb merged commit 11dc56b into yannickcr:master Oct 16, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 97.521%
Details
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: "'notOne' is missing in props validation",

This comment has been minimized.

@ghiculescu

ghiculescu Dec 4, 2019

possibly a dumb question, but why is this erroring? notOne doesn't appear anywhere else in this test?

This comment has been minimized.

@moroine

moroine Jan 28, 2020
Author Contributor

@ghiculescu it is at line 4791

This comment has been minimized.

@joeyparis

joeyparis May 28, 2021

@moroine I'm not sure I understand. Why is a separate test causing this test to fail? Aren't they each independent tests?

@levenecav
Copy link

@levenecav levenecav commented Jan 28, 2020

After this update, it stopped working for me. 7.16.0 - everything was fine, after 7.17.0 - below.

type PropsState = {|
    brokenExchangeConnection: boolean
|};

type PropsActions = {|
    updateUserParams: (string, StreamingUserParamsItem) => StreamingUpdateUserParamsAction
|};

type PropsParent = {|
    onTenorChange: (SettingsStreamerAccountsPair, StreamingUserParamsItem) => void
|};

type Props = {|
    ...PropsState,
    ...PropsActions,
    ...PropsParent
|};


51:31  error  'brokenExchangeConnection' PropType is defined but prop is never used  react/no-unused-prop-types
57:23  error  'updateUserParams' PropType is defined but prop is never used          react/no-unused-prop-types
70:20  error  'onTenorChange' PropType is defined but prop is never used             react/no-unused-prop-types
@moroine
Copy link
Contributor Author

@moroine moroine commented Jan 28, 2020

@levenecav could we have the code of the component?

@levenecav
Copy link

@levenecav levenecav commented Jan 28, 2020

He is big, but the essence of this:


import ...
import ...
import ...

type PropsState = {|
    brokenExchangeConnection: boolean
|};

type PropsActions = {|
    updateUserParams: (string, StreamingUserParamsItem) => StreamingUpdateUserParamsAction
|};

type PropsParent = {|
    onTenorChange: (SettingsStreamerAccountsPair, StreamingUserParamsItem) => void
|};

type Props = {|
    ...PropsState,
    ...PropsActions,
    ...PropsParent
|};

class Instrument extends PureComponent<Props> {
    render(): Node {
        return (
            <div
                data-marker={currency}
                className={styles.root}>
                {this.renderBody()}
            </div>
        );
    }

    renderBody(): Node {
        const { brokenExchangeConnection } = this.props;
        
        return (
            <div className={className}>
                <InstrumentErrorMessage
                    brokenExchangeConnection={brokenExchangeConnection}
                    onUpdateUserParams={this.handleUpdateUserParams}
                    onTenorChange={this.handleTenorChange} />
            </div>
        );
    }

    handleUpdateUserParams = () => {
        this.props.updateUserParams('id', { id: 'id', name: 'name' });
    };

    handleTenorChange = () => {
        this.props.onTenorChange('USD/RUB', { id: 'id', name: 'name' });
    };
}

export default SortableElement(connect(
    (state: StoreState, props: Props): PropsState => {
        const { brokenExchangeConnection } = state;

        return { brokenExchangeConnection };
    },
    ({
        updateUserParams: Actions.streaming.updateUserParams
    }: PropsActions)
)(Instrument));
@levenecav
Copy link

@levenecav levenecav commented Jan 28, 2020

I use "flow" for types.

@moroine
Copy link
Contributor Author

@moroine moroine commented Jan 28, 2020

If you use the following props do you have the same error? Because this PR is only about converting spread to the following props. Here I suspect the rule not finding usage of props in other methods

type Props = {|
    brokenExchangeConnection: boolean,
    updateUserParams: (string, StreamingUserParamsItem) => StreamingUpdateUserParamsAction,
    onTenorChange: (SettingsStreamerAccountsPair, StreamingUserParamsItem) => void,
|};
@levenecav
Copy link

@levenecav levenecav commented Jan 28, 2020

I have exactly the same problem as in #2138.
As far as I understand, this PR should solve this problem.
But in my case, on the contrary, this problem appeared, because in 7.16.0 it did not exist, and in 7.17.0 it appeared.

@moroine
Copy link
Contributor Author

@moroine moroine commented Jan 28, 2020

I'm sure your issue is caused by this PR. But this should be because now spreading is recognized as being

type Props = {|
    brokenExchangeConnection: boolean,
    updateUserParams: (string, StreamingUserParamsItem) => StreamingUpdateUserParamsAction,
    onTenorChange: (SettingsStreamerAccountsPair, StreamingUserParamsItem) => void,
|};

Could you check using Props without spreading with v7.16.0?

I suspect the error is due not used Props in the following function:

(state: StoreState, props: Props): PropsState => {
        const { brokenExchangeConnection } = state;

        return { brokenExchangeConnection };
    }
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jan 28, 2020

@levenecav Please file a new issue.

@moroine moroine deleted the moroine:feature/support-type-spread branch Jan 29, 2020
@levenecav
Copy link

@levenecav levenecav commented Jan 29, 2020

@moroine
Yes, you're right, I combined all props and the problem remains.
It turned out that the problem arises because getDerivedStateFromProps has a call to a function that is on the same level as the component class and in which I passed all props.
Thanks.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jan 29, 2020

Passing the props object around is an antipattern; always destructure what you need immediately.

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

Successfully merging this pull request may close these issues.

5 participants