-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD] [WIP] Remove usage of Network.post()
#17364
Conversation
…questWithSideEffects next
Network.post()
Network.post()
Hi, thanks for tagging me. It looks like all this code was just added a week ago in #16043. I'm kind of disappointed that this code got merged with all the set timeouts and As far as trying not to use
I have so many questions about the deep link component and I've asked them in the recent PR. |
Thanks @tgolen I think what I'll do is...
componentDidUpdate(prevProps) {
const didShortLivedTokenAppear = !prevProps.session.shortLivedAuthTokenForDeepLink.token && this.props.session.shortLivedAuthTokenForDeepLink.token;
const didShortLivedTokenErrorAppear = !prevProps.session.shortLivedAuthTokenForDeepLink.error && this.props.session.shortLivedAuthTokenForDeepLink.error;
if (didShortLivedTokenAppear) {...}
if (didShortLivedTokenErrorAppear) {...}
}
And maybe we can scrap the error handling part entirely - feels like that's handling an edge case where we somehow return an empty string for the token response - and can't really see how that would happen, but can try to do some testing. |
Ah actually I think we can't because the request could fail for some reason other than the API throwing - in that case we want to stop trying to do the deep link redirection and just render the app as normal. |
Network.post()
Network.post()
Hi, @marcaaron, sorry for the confusion. We don't need to deal with errors anymore. Since the redirection screen has been removed, it no longer makes sense to update Could you please review this new PR #17452 together?
|
Ah, love this. I came to this comment a little too late though, so you can ignore some of my comments in #17452 |
Closing for now based on the comment here. |
Details
Trying to clean up remaining usages of
Network.post()
so we can fully deprecate the old APIThis is touching some interesting code called the
DeeplinkWrapper
. As far as I can tell...Network.post()
directly instead ofAPI.makeRequestWithSideEffects()
orAPI.read()
setTimeout()
dance (I honestly don't really get this part)So my questions are...
Can we avoid this usage of
makeRequestsWithSideEffects()
somehow?I think if we try to use Onyx to retrieve the short lived token things would become even weirder and harder to understand...
cc @tgolen to see if he has any ideas...
Fixed Issues (Another small step towards finishing the Network migration)
https://github.com/Expensify/Expensify/issues/208281
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android