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

Feat: Move FlatLists to @react-native/flat-lists package #35423

Closed

Conversation

Pranav-yadav
Copy link
Contributor

@Pranav-yadav Pranav-yadav commented Nov 21, 2022

Summary

This PR moves FlatList, SectionList and related files (tests, etc.) to a new package @react-native/flat-lists located under packages/flat-lists as proposed in #35263
This is a first step towards moving all the FlatList-related code to a separate package.
This will allow us to make changes to the FlatList implementation without affecting the rest of the React Native codebase.

Changelog

[General] [Changed] - Move FlatLists to @react-native/flat-lists package.
FlatList and SectionList are now available in @react-native/flat-lists package.
Warning: This is a breaking(?) change, and you may need to update your imports to use the new package.

Test Plan

  • ensure yarn lint && yarn flow && yarn test is #00ff00

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2022
@analysis-bot
Copy link

analysis-bot commented Nov 21, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,520,731 +699
android hermes armeabi-v7a 7,836,751 +822
android hermes x86 9,000,706 +858
android hermes x86_64 8,855,902 +729
android jsc arm64-v8a 9,141,696 +275
android jsc armeabi-v7a 8,333,645 +287
android jsc x86 9,196,198 +282
android jsc x86_64 9,454,318 +285

Base commit: 928f4fb
Branch: main

@analysis-bot
Copy link

analysis-bot commented Nov 21, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: d9ade19
Branch: main

@pull-bot
Copy link

PR build artifact for 083b044 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 083b044 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 083b044 to de2c5be Compare November 21, 2022 18:18
@Pranav-yadav

This comment was marked as resolved.

@pull-bot
Copy link

PR build artifact for de2c5be is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for de2c5be is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

Have a look at the feedback on the VirtualizedList PR. Specifically:

  • We should not be reaching inside any packages. Instead @react-native/flat-lists should export everything that can be imported, and it should import from react-native and not internal paths.
  • This package should have the virtualized-lists package as a dependency, rather than creating circular dependencies.

#35406

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch 2 times, most recently from 17ce59e to c3302ac Compare November 22, 2022 16:43
@pull-bot
Copy link

PR build artifact for 17ce59e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 17ce59e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for c3302ac is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for c3302ac is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from c3302ac to 0074ab5 Compare November 22, 2022 18:21
@pull-bot
Copy link

PR build artifact for 0074ab5 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 0074ab5 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Pranav-yadav

This comment was marked as resolved.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 0074ab5 to 6103302 Compare November 23, 2022 12:59
@pull-bot
Copy link

PR build artifact for 6103302 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 6103302 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 6103302 to dcb0a87 Compare November 23, 2022 14:24
@pull-bot
Copy link

PR build artifact for dcb0a87 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for dcb0a87 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

packages/flat-lists/package.json Outdated Show resolved Hide resolved
packages/flat-lists/package.json Outdated Show resolved Hide resolved
@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch 2 times, most recently from 2429a77 to d67acb3 Compare November 23, 2022 15:47
@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch 2 times, most recently from a6ce399 to 9e18d0a Compare February 7, 2023 18:49
@Pranav-yadav Pranav-yadav requested review from necolas and removed request for cortinico February 8, 2023 11:57
@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Feb 8, 2023

@NickGerleman / @hoxyq I have rebased this PR on main as the @react-native/virtualized-lists commit were merged.
Requesting another round of review.

Update:

  • rebased on main
  • fixed some typescripts errors

cc: @necolas
PS: Since this PR is being tracked under umbrella issue(s) you may add Tech: Monorepo label ...

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Feb 14, 2023

@necolas re-pinging, in case you missed.. 🙂

Edit: cc @cortinico

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Feb 26, 2023

@NickGerleman please let me know if should rebase again so that you can review it afterwards.
PS: I just don't want to waste this much effort put into refactoring it.

I know this might not be prioritized but, I don't have any idea about it as well; (no one is replying), hence pinging again.
Sorry for the re-pings.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 9e18d0a to eb493e8 Compare February 27, 2023 19:04
@Pranav-yadav Pranav-yadav requested a review from necolas February 27, 2023 19:40
@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch 2 times, most recently from b4fc24f to 0b9a654 Compare March 22, 2023 11:21
@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 0b9a654 to 94c33d8 Compare March 22, 2023 12:00
@NickGerleman
Copy link
Contributor

Sorry I haven't been able to take a close look at this (though it seems like you found some help on the Discord). Though as a general bit of feedback, I'm not sure it makes sense to add a new plural flat-lists package, when we have one FlatList, vs adding to the existing plural virtualized-lists package, which currently also only has one list.

@necolas
Copy link
Contributor

necolas commented Mar 22, 2023

They both also have the section list variant

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Mar 22, 2023

Sorry I haven't been able to take a close look at this.

No worries :)

I'm not sure it makes sense to add a new plural flat-lists package.

I did consider that at the start only but, it's not a FlatList only package; virtualized-lists and flat-lists have SectionList variants.
and specifically flat-lists has:

  1. SectionList,
  2. SectionListModern

variants as well.. as Nicolas said.
So I think plural seems logical here, yet it should have been singular only, iff SectionList had been isolated earlier...

They both also have the section list variant

Yeah.

@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch 2 times, most recently from 6b4891a to 600c5af Compare March 23, 2023 14:05
Summary: This diff moves `FlatList` and `SectionList` to a
new package `@react-native/flat-lists`.
This is a first step towards moving all the `FlatList`-related code
to a separate package.
This will allow us to make changes to the `FlatList` implementation
without affecting the rest of the React Native codebase.

Changelog: [General] [Changed] - Move `FlatList`s to `@react-native/flat-lists` package.
`FlatList` and `SectionList` are now available in `@react-native/flat-lists` package.
**Warning**: This though this is NOT a breaking change, and you will NOT need to
update your imports to use the new package. But, moving forward it will
be better to install `@react-native/flat-list` if you want to import explicitly.
@Pranav-yadav Pranav-yadav force-pushed the feat/add-flat-lists-package branch from 600c5af to 7184780 Compare March 23, 2023 14:48
@Pranav-yadav
Copy link
Contributor Author

🎉 All Green !!

Will anyone import this?

@NickGerleman

*/

import typeof ScrollViewNativeComponent from 'react-native';
import type {ViewStyleProp} from 'react-native/Libraries/StyleSheet/StyleSheet';
Copy link
Contributor

@necolas necolas Mar 23, 2023

Choose a reason for hiding this comment

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

Please can we avoid having any imports from RN internals in this package

Copy link
Contributor Author

@Pranav-yadav Pranav-yadav Mar 24, 2023

Choose a reason for hiding this comment

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

Yeah. I forgot that; this way it may not work in external projects.

Right now I'm in college, will try to change it once I reach home.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. I'm busy with assignment tests at college, will try to look at this; tomorrow eod..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we avoid having any imports from RN internals in this package

@necolas in order to avoid all of them, we'll need to export some other modules from react-native such as,

  • deepDiffer

@NickGerleman Are there any issues against that? If no issues, then should I open separate PR(s) for those changes or it is fine to do it in this PR only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, those types are not exported from react-native
image

So, they will need to be exported, in order to directly import??

@roryabraham
Copy link

roryabraham commented Jun 14, 2023

Apologies if this has been discussed already, but might it be simpler to just include these files in the @react-native/virtualized-lists package? Is there any advantage to keeping them separate?

Edit: Oh, this is the same feedback @NickGerleman had above: #35423 (comment)

@Pranav-yadav
Copy link
Contributor Author

Theoretically "Yes". But, both lists were implemented and exist for a special (different) purpose, keeping them separate would support the initial cause of "separation of concerns".

If we combine them into a "single" package:

  1. Sine, both have SectionList variants it could be confusing.
  2. Would significantly increase the package size. (unwanted)
  3. There is possibility that some types may collide.

WDYT?

P.S.: Personally, I would've loved to have them in a single package named @react-native/lists (or similar) in the first place but, doing this change now would need a breaking release while keeping it backward compatible.

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 12, 2023
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants