Skip to content

/ react-native

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

Jump to bottom

Migrate off of old context APIs #28103

Open
TheSavior opened this issue Feb 18, 2020 · 11 comments
Open

Migrate off of old context APIs #28103

TheSavior opened this issue Feb 18, 2020 · 11 comments
Labels
Good first issue JavaScript

Comments

@TheSavior
Copy link
Member

@TheSavior TheSavior commented Feb 18, 2020 •
edited

We want to get rid of our usage of the prop-types package. The last remaining usage of this is a couple of old components that use the old React context API. We need to migrate these components to use the modern React.createContext API.

The components using the old context API are:

For more information about the old API and how to migrate to the new API you can check out the React docs for Context: https://reactjs.org/docs/context.html#classcontexttype

If we are able to remove the usage of prop-types from React Native we should be able to substantially lower the bundle size of React Native. We'd love your help!
@safaiyeh safaiyeh mentioned this issue Feb 18, 2020
Open
@Naturalclar

This comment has been minimized.

Sign in to view
Copy link
Contributor

@Naturalclar Naturalclar commented Feb 18, 2020

I'd love to help out! Could I work on Modal?
@venits

This comment has been minimized.

Sign in to view
Copy link
Contributor

@venits venits commented Feb 18, 2020

I would love to take care of Incremental :)
@venits venits mentioned this issue Feb 18, 2020
Closed
@mikaoelitiana

This comment has been minimized.

Sign in to view
Copy link
Contributor

@mikaoelitiana mikaoelitiana commented Feb 18, 2020

hey there, I can check virtualizedlist
facebook-github-bot added a commit that referenced this issue Feb 19, 2020
@venits @facebook-github-bot
- Remove leftover `Incremental` component (#28107)
Loading status checks…
e998002 
Summary:
1/4 of #28103

## Changelog

[JavaScript][Removed] - Remove leftover `Incremental` component.
Pull Request resolved: #28107

Test Plan: In RNTester `Incremental` component is no longer used so I modified one of existing examples to use it.

Reviewed By: rickhanlonii

Differential Revision: D19960536

Pulled By: TheSavior

fbshipit-source-id: 791bda7138ac23916957577ed5f5c465c5e96299
@thymikee

This comment has been minimized.

Sign in to view
Copy link
Collaborator

@thymikee thymikee commented Feb 24, 2020

@Naturalclar @mikaoelitiana they're all yours to take :)
@mikaoelitiana

This comment has been minimized.

Sign in to view
Copy link
Contributor

@mikaoelitiana mikaoelitiana commented Feb 24, 2020

@thymikee thanks, I am already working on VirtualizedList, the module is quite big and I try to make sure I am doing things the right way
@Naturalclar Naturalclar mentioned this issue Feb 27, 2020
Open
@TheSavior

This comment has been minimized.

Sign in to view
Copy link
Member Author

@TheSavior TheSavior commented Mar 2, 2020

It looks like Modal actually only uses the VirtualizedList Context and AppContainer Context, it doesn't have its own context. So we need to get the VirtualizedList and AppContainer ones done first.
@mikaoelitiana

This comment has been minimized.

Sign in to view
Copy link
Contributor

@mikaoelitiana mikaoelitiana commented Mar 5, 2020

So this means these contexts need to be exported, right?
@TheSavior

This comment has been minimized.

Sign in to view
Copy link
Member Author

@TheSavior TheSavior commented Mar 5, 2020

Yeah, the root tag context is already exported, unstable_RootTagContext, the VirtualizedList one can be exported as a static property on VirtualizedList: VirtualizedList.Context
@thymikee

This comment has been minimized.

Sign in to view
Copy link
Collaborator

@thymikee thymikee commented Mar 7, 2020

@TheSavior is the VirtualizedList context used internally at FB? It looks like Modal overwrites the virtualizedList context with null, so it doesn't propagate further and there are no consumers of this inside RN codebase. So, if there are no internal FB components using this context, we could maybe kill it entirely? But I guess there are 😄
@TheSavior

This comment has been minimized.

Sign in to view
Copy link
Member Author

@TheSavior TheSavior commented Mar 7, 2020

Yeah, these contexts are all used fairly heavily. Any kind of portal type thing would use it like Modal or BottomSheet.
osdnk added a commit to osdnk/react-native that referenced this issue Mar 9, 2020
@venits @osdnk
- Remove leftover `Incremental` component (facebook#28107)
782e88c 
Summary:
1/4 of facebook#28103

## Changelog

[JavaScript][Removed] - Remove leftover `Incremental` component.
Pull Request resolved: facebook#28107

Test Plan: In RNTester `Incremental` component is no longer used so I modified one of existing examples to use it.

Reviewed By: rickhanlonii

Differential Revision: D19960536

Pulled By: TheSavior

fbshipit-source-id: 791bda7138ac23916957577ed5f5c465c5e96299
@joelbrewer

This comment has been minimized.

Sign in to view
Copy link

@joelbrewer joelbrewer commented Mar 24, 2020

It appears all of the tasks on this issue have been claimed? Looking for my first RN issue to help out on :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good first issue JavaScript
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
@TheSavior @Naturalclar @venits @mikaoelitiana @thymikee @joelbrewer
You can’t perform that action at this time.