Convert network overlay to use flatlist #21837
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
AlanFoster
force-pushed
the
convert-network-overlay-to-use-flatlist
branch
from
f2487aa to
d3073f5
Compare
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| const networkRequestInfo = requests[socketIndex]; | ||
| networkRequestInfo.status = statusCode; | ||
| networkRequestInfo.closeReason = closeReason; | ||
| return {requests}; |
There was a problem hiding this comment.
I'm not a massive fan of the localised state mutation, but it feels like an improvement to the existing code.
I assumed the previous mutation was for performance reasons, and I've aligned with the most simplistic approach for now.
AlanFoster
force-pushed
the
convert-network-overlay-to-use-flatlist
branch
from
d3073f5 to
c09fc6d
Compare
|
Thanks for this! I was just looking today at how this is one of the only callsites of ListView in the repo. Unfortunately, this PR is huge and makes it hard to read through and see what changes are necessary to each piece. Can you break this into a couple different changes?
Each of these changes seem like they could be done in isolation. It would be greatly appreciated if you could make it as easy as possible to review your changes. We definitely want these things to land. |
|
@TheSavior I agree - it's quite the PR. The code in the file was previously quite complected, but thankfully it's a single component still!
The main responsibilities are:
I can try to extract this into two separate commits, but the commits wouldn't be atomic as they wouldn't work in isolation without lots of extra glue code to help with an intermediate step, which might make the review more confusing! Let me know if the additional insights make this PR easier to review, or if you'd still like two separate commits :] |
|
Thanks for that explanation. I'm not sure I understand why the changes to network handling logic / setState are deeply coupled to ListView/FlatList. Can you help me understand this better? It seems like it should be possible to migrate to FlatList without also migrating the business logic very much. |
@TheSavior That was my initial plan too, but once I learnt what the existing code was doing - it ended up easier to modify the code to be closer to idiomatic React. Let me know your thoughts after you've had a chance to review it 👍 |
|
I tried to import the PR to test it manually but noticed that your target branch is |
AlanFoster
force-pushed
the
convert-network-overlay-to-use-flatlist
branch
from
c09fc6d to
60a222b
Compare
AlanFoster
requested review from
grabbou,
hramos,
janicduplessis,
Kureev,
mhorowitz and
shergin
as code owners
|
@TheSavior Done! Apologies :) |
AlanFoster
force-pushed
the
convert-network-overlay-to-use-flatlist
branch
from
60a222b to
375c8c2
Compare
There was a problem hiding this comment.
TheSavior has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@AlanFoster merged commit efa6016 into |
Summary: Pull Request resolved: facebook#21837 Reviewed By: TheSavior Differential Revision: D10449940 Pulled By: RSNara fbshipit-source-id: 28dcd5906c64070ef50867425ae7517d391300e2
Test Plan:
I ran through the following scenarios:
Release Notes:
Help reviewers and the release process by writing your own release notes. See below for an example.
[GENERAL] [ENHANCEMENT] [Network Inspector] - The network inspector has been updated to use FlatList, as ListView has been marked as deprecated.