-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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 virtualized lists to @react-native/virtualized-lists #35406
feat: Move virtualized lists to @react-native/virtualized-lists #35406
Conversation
c2dfa4a
to
b942261
Compare
Base commit: 1fef376 |
Base commit: 621969b |
PR build artifact for b942261 is ready. |
PR build artifact for 59fea61 is ready. |
@cortinico and @necolas, any thoughts on how to set up |
Right now I am using Verdaccio and publishing necessary packages to local proxy. You can check how I did it in this commit. I am currently working on first phase of monorepo project #34692, this should unblock you after we will merge my proposed changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Would also like some feedback from @necolas, but my sense is that we shouldn't extract to a separate package if both packages are still relying on each others private APIs. The package would not yet be reusable (even across different versions of RN), and the boundaries between the two are not enforced.
Copying the set of internal transitive dependencies to VirtualizedList so it doesn't rely on the internals seems like a potential start for migrating though, and it would allow us to shim between different platforms more easily in the VirtualizedList package.
1f3dec5
to
d689da0
Compare
PR build artifact for d689da0 is ready. |
PR build artifact for 518cf03 is ready. |
packages/rn-tester/js/examples/Experimental/PlatformTest/RNTesterPlatformTestResultView.js
Outdated
Show resolved
Hide resolved
dd308f1
to
536e6a0
Compare
536e6a0
to
b52ee19
Compare
@NickGerleman any updates on this? I know that this impacts a bunch of internal Meta code that references old VirtualizedList paths, but I was wondering, is there something I can help with? |
I haven't been able to dedicate much time to pushing this internally. I spent a day a while back trying to find everywhere the previous structure was relied on, and I don't think we could safely ship the refactoring without staging it a bit more. Like, leaving old files around which forward to the new ones, so we can change product code more piecemeal. It looked like a weekish of work dedicated to internal cleanup. Because it's all Meta code, we will need someone on our side to do the work, and figure out how to stage. @hoxyq has worked through some similar headaches with lean core modules, and also has been looking at monorepo work. Might have a take on if there is a way we could stage this, where we could move to this structure in OSS without ripping up too much code out there. |
Will take a look at it next week |
@hoxyq has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Isn't this what the PR does now? Maybe I missed something? |
I think the problem is that there are internal projects importing from specific internal paths like |
Yeah, amazingly enough, there were even multiple cases of product code including the VirtualizedList utils helper to get a I remember @lunaleaps was looking at what it would take to enforce the public boundaries more. |
…book#35406) Summary: This PR moves `VirtualizedList`, `VirtualizedSectionList`, and its files to a separate package called `react-native/virtualized-lists` located under `packages/virtualized-lists` as proposed on facebook#35263 ## Changelog [General] [Changed] - Move virtualized lists to react-native/virtualized-lists package Pull Request resolved: facebook#35406 Test Plan: 1. Open the RNTester app and navigate to `FlatList` or `SectionList` page 2. Test virtualized lists through the many sections https://user-images.githubusercontent.com/11707729/202878843-2b1322f5-cfee-484e-aaf3-d8d4dc0b96cc.mov Differential Revision: D41745930 Pulled By: hoxyq fbshipit-source-id: 67a971a3f06895bce360a7463c66a6b1f334f46b
So I've made some changes both internally and in frames of this PR (exported in #36035), this is now accepted by @NickGerleman Planning to merge this PR early next week 🤞 Meanwhile you can grep the difference between this PR and #36035 to check if everything is expected There should be not that much, mostly re-exporting something, most of the work was made on the internal side |
Thanks for awesome work @hoxyq! Should I cherry-pick your commit to this PR? |
No, this will be automatically resolved when I will merge internal changes, your commit will be merged in main branch together with mine and this PR will be closed and marked as merged |
…book#35406) Summary: This PR moves `VirtualizedList`, `VirtualizedSectionList`, and its files to a separate package called `react-native/virtualized-lists` located under `packages/virtualized-lists` as proposed on facebook#35263 ## Changelog [General] [Changed] - Move virtualized lists to react-native/virtualized-lists package Pull Request resolved: facebook#35406 Test Plan: 1. Open the RNTester app and navigate to `FlatList` or `SectionList` page 2. Test virtualized lists through the many sections https://user-images.githubusercontent.com/11707729/202878843-2b1322f5-cfee-484e-aaf3-d8d4dc0b96cc.mov Reviewed By: cipolleschi Differential Revision: D41745930 Pulled By: hoxyq fbshipit-source-id: d3d33896801fd69448c6893b86fd5c2363144fd0
Summary
This PR moves
VirtualizedList
,VirtualizedSectionList
, and its files to a separate package called@react-native/virtualized-lists
located underpackages/virtualized-lists
as proposed on #35263Changelog
[General] [Changed] - Move virtualized lists to @react-native/virtualized-lists package
Test Plan
FlatList
orSectionList
pageScreen.Recording.2022-11-19.at.22.46.13.mov