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

BREAKING CHANGE: Use React.forwardRef for all components #499

Merged
merged 38 commits into from
Oct 13, 2021

Conversation

youchann
Copy link
Contributor

@youchann youchann commented Sep 27, 2021

close: #495

https://ja.reactjs.org/docs/forwarding-refs.html

コンポーネントライブラリの中で、forwardRef を使い始めた場合、破壊的変更として扱い、新しいメジャーバージョンをリリースすべきです。ライブラリが外から見て今までと違う挙動(例えば、どの値が ref に代入されるかや、どの型がエクスポートされるのか)をする可能性があり、古い挙動に依存しているアプリケーションや他のライブラリを壊す可能性があるからです。

よってこれは破壊的変更となる。

@youchann youchann added enhancement New feature or request breaking change labels Sep 27, 2021
@youchann youchann self-assigned this Sep 27, 2021
@netlify
Copy link

netlify bot commented Sep 27, 2021

✔️ Deploy Preview for ingred-ui ready!

🔨 Explore the source changes: 9d7e71e

🔍 Inspect the deploy log: https://app.netlify.com/sites/ingred-ui/deploys/61662aedb4f7670007fe67c2

😎 Browse the preview: https://deploy-preview-499--ingred-ui.netlify.app

Comment on lines +89 to +92
// FIXME: Imprement without type assertion
export default React.forwardRef(CreatableSelect) as <T>(
props: CreatableSelectProps<T> & { ref?: React.ForwardedRef<HTMLDivElement> },
) => ReturnType<typeof CreatableSelect>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generics使うタイプのコンポーネントはこんな書き方にしている
ref: https://fettblog.eu/typescript-react-generic-forward-refs/

他で言うと<Select />, <DataTable />

</Flex>
);
};
const ErrorText = React.forwardRef<HTMLDivElement, ErrorTextProps>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

基本的にはこんな感じでforwardRef()でラップするだけ

Comment on lines +49 to +62
const ExportedComponent = NavigationRail as any;
ExportedComponent.Container = NavigationRailContainer;

NavigationRail.Container = NavigationRailContainer;
NavigationRail.Header = Header;
NavigationRail.Content = Content;
NavigationRail.Footer = Footer;
NavigationRail.Menu = Menu;
NavigationRail.ExpantionMenu = ExpantionMenu;
NavigationRail.ExpantionMenuItem = ExpantionMenuItem;
NavigationRail.Fixture = Fixture;
NavigationRail.MainContent = MainContent;
ExportedComponent.Container = NavigationRailContainer;
ExportedComponent.Header = Header;
ExportedComponent.Content = Content;
ExportedComponent.Footer = Footer;
ExportedComponent.Menu = Menu;
ExportedComponent.ExpantionMenu = ExpantionMenu;
ExportedComponent.ExpantionMenuItem = ExpantionMenuItem;
ExportedComponent.Fixture = Fixture;
ExportedComponent.MainContent = MainContent;

export default NavigationRail;
export default ExportedComponent as ExportedComponentType;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.hogehogeで提供するタイプのコンポーネントは新たに型作ってtype assertした

| string
| null
| undefined;

export function useMergeRefs<T>(...refs: ReactRef<T>[]): ReactRef<T> {
export function useMergeRefs<T>(...refs: ReactRef<T>[]): React.Ref<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ところどころ使っていて型が合わなかったので調整した

@kohashi kohashi changed the title Use React.forwardRef for all components BREAKING CHANGE: Use React.forwardRef for all components Oct 12, 2021
Copy link
Contributor

@kohashi kohashi left a comment

Choose a reason for hiding this comment

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

これ見るの忘れてた!
全体的に ref を使用して DOM参照渡せるようにする、良さそうかと思いますー。

as={children == null ? "span" : "label"}
disabled={disabled}
size={radioButtonSize}
size={size}
>
<input
Copy link
Contributor

@kohashi kohashi Oct 12, 2021

Choose a reason for hiding this comment

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

<input ref={ref} type="radio" でなく、上位要素の <Wrapper> にrefを渡してる(渡す?取る、と言うべき?)。

これはMUIもそんな感じで、最上位に ref を渡している。
個別に深いものを渡したいケースで別途渡すケースもある。 ここの <Input ... ref={handleInputRef}> のように
https://github.com/mui-org/material-ui/blob/512896973499adbbda057e7f3685d1b23cc02de9/packages/mui-material/src/InputBase/InputBase.js#L503-L535

ok.

@@ -220,4 +221,7 @@ const Select = <T,>(
);
};

export default Select;
// FIXME: Imprement without type assertion
Copy link
Contributor

Choose a reason for hiding this comment

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

Imprement ??? Implementのことかな? ( nitpick)

@youchann
Copy link
Contributor Author

PR名の変更ありがとうございますmm(この辺のトンマナ?にまだ詳しくなくて...)

@youchann youchann merged commit d7e84a8 into master Oct 13, 2021
@youchann youchann deleted the use-forward-ref-for-all-components branch October 13, 2021 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use React.forwardRef for all components
2 participants