using environment variable set by Environment.js for ios/android#12546
using environment variable set by Environment.js for ios/android#12546smrutiparida wants to merge 3 commits into
Conversation
mountiny
left a comment
There was a problem hiding this comment.
@smrutiparida Would you be able to also include screenshots/videos of the platforms confirming everything works as expected and ideally showcase the QA steps with the console.debug output showing this is working as expected in your local environment?
Thanks for raising this PR quickly.
mountiny
left a comment
There was a problem hiding this comment.
@smrutiparida The code looks fine to me but could you please included screen recording of the console with debug and whats in the .env so we can see this works for you as expected.
Then you can fill in the PR author checklist for other platforms too so it passes
Done now. Please see the Android video |
|
recheck |
|
Thank you very much @smrutiparida fixed the PR author checklist. Gonna wait a bit for the review from Rajat |
|
Thanks for waiting. I will try to get on it. If not, I will inform the same. |
|
@parasharrajat let me know if you can get to this on Monday, if not, feel free to use the two new C+ to re-assign for testing/review. Thanks! |
|
I will be back in 2 hours and review this. |
|
Back so checking now. |
|
Should QA steps 3 and 4 call the development API? @mountiny |
|
@parasharrajat I think QA 4 is production right? QA3 seems like development, I can confirm this works. |
|
Under some discussion... |
|
@parasharrajat can you point to the discussion if it is other than the one in the Slack? Other than this https://expensify.slack.com/archives/C01GTK53T8Q/p1668446333318199 which seems resolved. |
| // Additionally, for iOS/android betaChecker.isBetaBuild() does the version check | ||
| // environment is initialized as null as the default set in CONFIG.js always takes precedence | ||
| let environment = null; | ||
| Environment.getEnvironment() |
There was a problem hiding this comment.
Does this function read a different Config from what we have in env which was used to build the app?
There was a problem hiding this comment.
I still do not get it. Why getting the value here is different from Config.js file?
There was a problem hiding this comment.
For web/desktop the function reads the config as you said in libs/Environment/getEnvironment/index.js.
function getEnvironment() {
return Promise.resolve(lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV));
}
For Native, the function in libs/Environment/getEnvironment/index.native.js additionally does a isBetaBuild() check. See below. The isBetaBuild compares the pkg.version with the Google Play Store version and accordingly sets isBeta flag. iOS does the same check using NativeModules.EnvironmentChecker check for iOS devices.
if (lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV) === CONST.ENVIRONMENT.DEV) {
environment = CONST.ENVIRONMENT.DEV;
return resolve(environment);
}
// If we haven't set the environment yet and we aren't on dev, check to see if this is a beta build
betaChecker.isBetaBuild()
.then((isBeta) => {
environment = isBeta ? CONST.ENVIRONMENT.STAGING : CONST.ENVIRONMENT.PRODUCTION;
resolve(environment);
});
There was a problem hiding this comment.
@smrutiparida Thanks for the explanation, can you also include this in the PR body?
There was a problem hiding this comment.
Ok, I got it. Yeah, please add it to the details. I read all of the comments on the issue and PR, it does not explain why are we doing this change and how this works.
| // getEnvironment promise returns the environment based on ENVIRONMENT Config for web/desktop. | ||
| // Additionally, for iOS/android betaChecker.isBetaBuild() does the version check | ||
| // environment is initialized as null as the default set in CONFIG.js always takes precedence |
There was a problem hiding this comment.
| // getEnvironment promise returns the environment based on ENVIRONMENT Config for web/desktop. | |
| // Additionally, for iOS/android betaChecker.isBetaBuild() does the version check | |
| // environment is initialized as null as the default set in CONFIG.js always takes precedence | |
| // `getEnvironment` returns the environment based on .env Config for web/desktop. | |
| // Additionally on iOS/android beta is checked to get the runtime Environment. |
| // environment is initialized as null as the default set in CONFIG.js always takes precedence | ||
| let environment = null; | ||
| Environment.getEnvironment() | ||
| .then((env) => { |
There was a problem hiding this comment.
There is a problem with this change. It might not work always. requests are made in promises and you are trying to get the env info outside the promise chain.
It is possible that for some requests ENV is not yet.
There was a problem hiding this comment.
Do you suggest to then await like below.
const environment = await Environment.getEnvironment();
The only issue with await was it shall make the xhr execution wait in HttpUtils till promises returns a value. This may slow down the app starting in android environment since beta check is a network call.
In compare to that "let" also introduces a temporal dead zone within the block execution. So I think we should be ok with null in staging during environment initialization period.
There was a problem hiding this comment.
I think we are not using await in our codebase.
When is this being called @smrutiparida? This is called just once and before being resolved it is null, right? In that case we will use the default which is production?
What if this is in dev, that is not influenced and we will use whatever is in the .env right?
There was a problem hiding this comment.
When is this being called @smrutiparida? This is called just once and before being resolved it is null, right? In that case we will use the default which is production?
Yes, we will fall back to production API_URL
What if this is in dev, that is not influenced and we will use whatever is in the .env right?
Yes.
One note here is that presently .env do not have STAGING_EXPENSIFY_URL and STAGING_SECURE_EXPENSIFY_URL configuration. The present values are hard-coded in Config.js. We need to init the values in .env in order to use them in dev environment.
There was a problem hiding this comment.
Yes, we will fall back to production API_URL
This will have to reset everytime the app is killed and opened again, right? If I just put it in a background and come back the value will be still "cached"
There was a problem hiding this comment.
@parasharrajat can you elaborate more, I think we trying to avoid adding middlewares so there would have to be strong reason for this.
There was a problem hiding this comment.
I was thinking that we create a Middleware that switches the URL based on the ENV. But that seems not possible. Our Middlewares apply after the request is sent to the server.
@smrutiparida Could you please share some suggestions on what are the options?
Yes, we will fall back to production API_URL
How? the local environment variable is initialized with null value. Until the promise is resolved it will be null.
Thus, if shouldUseStagingServer is set, requests will go to staging even if a user is using the PROD app. This sounds wrong. Right?
There was a problem hiding this comment.
How? the local environment variable is initialized with null value. Until the promise is resolved it will be null.
Thus, if shouldUseStagingServer is set, requests will go to staging even if a user is using the PROD app. This sounds wrong. Right?
No, with below logic user can only be in staging with envrionment set to staging.
if (environment === CONST.ENVIRONMENT.STAGING && shouldUseStagingServer) {.... }
There was a problem hiding this comment.
Oh Correct. I don't know why I got confused.
There was a problem hiding this comment.
shouldUseStagingServer should never be set to truthy value in prod app, the toggle does not exist in there I believe.
|
@parasharrajat @smrutiparida So how are we feeling on this one? I think we should decide here. If you think we should not merge this, can you please post a summary of the reasons why not to do so. For now it just feels the only problem would be that for short period of time in staging app we would point to production, but given that should be short time at app start it does not feel like a blocker if other solutions involve changing our entire deployment pipeline. |
|
I am not sure too. If we are fine with a short delay, its fine to merge it. I will test it to determine if this can be a real problem. There are two issues
Explanation of 2: Because we are making an API request in the global scope, it will be called as soon as the app is bootup. Js is single-threaded, thus even though HTTP calls are async, it will have some impact. |
But since we dont wait for the response this impact should be really small right.
I think in general we should not be in state where calling prod api from staging app should be a problem. This should work just fine in 99.9% cases. Is there potentially any other way to handle this without the promise. For example how is the badge added to the app (DEV, Staging), that is also coming from same Promise I assume. I think we could move this promise after app is loaded, the staging app should work just fine with production API so if we would switch this after the app boots up, I dont think it would be too harmful. But again, is there no better solution? @smrutiparida you spent lots of time looking into this, any ideas? We dont necesarily need to follow what was in the proposal. |
Yes, but they are hooked to the component lifecycle. Components are started after the app is bootup.
Yes, but it is adding up to the existing code execution. We don't have to be too much concerned about it. It won't have much impact. Mainly, if we have a better solution let's do that. |
I am up for better solutions, how could we do that? |
|
Sorry, I wanted to say if we have one. Mistype 😄 |
|
😂 got it |
|
I think this solution is fine for now. What I would change is try to move the promise to get the environment somewhere later in the app lifecycle so it does not slow down the boot time. @smrutiparida can you think of a way to achieve that. |
|
|
I feel like this is tricky, we should avoid adding something which will be blocking the boot time. I think that now when we have a better idea of the context and potential drawbacks, we should bring this up to Slack for discussion with more engineers. Unfortunately, I will be mostly ooo tomorrow. The issue I see here is that while this solves the issue it is at cost of a promise which is executed also on production App which is unnecessary load in that case and we focus on performance mainly right now. Would anyone be able to summarize this context and post in Slack open-source please? cc @smrutiparida? |
|
Happy to review the other PR. |
@mountiny @parasharrajat @AndrewGable
Details
The issue occured as a regression for ios/android apps for PR # 10690. Despite setting shouldUseStagingServer preference by the User, the staging URL was not getting picked up by the apiRoot to call the API in ios and android staging builds.
The reason for this was, ENVIRONMENT variable from .env config is used to set IS_STAGING variable. For ios/android this variable was always set as PRODUCTION due to the present Fastlane build process
The fix was to dynamically deduce the environment by calling Environment.getEnvironment() promise instead of using IS_STAGING variable.
For web/desktop the function reads the config as you said in libs/Environment/getEnvironment/index.js.
For Native, the function in libs/Environment/getEnvironment/index.native.js additionally does a isBetaBuild() check. See below. The isBetaBuild compares the pkg.version with the Google Play Store version and accordingly sets isBeta flag. iOS does the same check using NativeModules.EnvironmentChecker check for iOS devices.
Fixed Issues
$ #11561
Tests
console.debug(shouldUseStagingServer is ${shouldUseStagingServer}, environment is ${environment} and apiRoot is ${apiRoot});code in the changed code.Expectation is that the environment based apiRoot shall be logged in console.
QA Steps
Settings -> Preference -> Use staging server = ON along with ENVIRONMENT=staging in .env
- Open app on web
- Go to Settings -> Preference
- Make the Use staging server switch ON
- Open dev tool
- Go to Network tab.
- Focus on app and send some message.
- Verify that the request in Fetch/XHR is of staging server https://staging.expensify.com or https://staging-secure.expensify.com.
Settings -> Preference -> Use staging server = OFF along with ENVIRONMENT=staging and USE_WEB_PROXY=true in .env
- Open app on web
- Go to Settings -> Preference
- Make the Use staging server switch OFF
- Open dev tool
- Go to Network tab.
- Focus on app and send some message.
- Verify that Fetch/XHR is of production server (without staging prefixes) to / ( ideally it should have gone to new.expensify.com by with useWebProxy it will be sent to / to avoid CORS error)
Settings -> Preference -> Use staging server = ON along with ENVIRONMENT=development in .env
- Open app on web
- Go to Settings -> Preference
- Make the Use staging server switch OFF
- Open dev tool
- Go to Network tab.
- Focus on app and send some message.
- Verify that Fetch/XHR is of production server (without staging prefixes) https://www.expensify.com or https://secure.expensify.com.
Settings -> Preference -> Use staging server = OFF with no .env file
- Open app on web
- Go to Settings -> Preference
- Make the Use staging server switch OFF
- Open dev tool
- Go to Network tab.
- Focus on app and send some message.
- Verify that Fetch/XHR is of production server (without staging prefixes) to / ( ideally it should have gone to new.expensify.com by with useWebProxy it will be sent to / to avoid CORS error)
Settings -> Preference -> Use staging server = ON along with ENVIRONMENT=production in .env
- The Preference will not be visible in the UI
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
Test Case 1:
Test Case 2:
Mobile Web - Chrome
Test Case 2:
screenrecord.mov
Mobile Web - Safari
Test Case 4:
Desktop
TestCase 4:
iOS
Android
TestCase 1:
Part 1 video
part1.mov
Part 2 video
part2.mov