Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

HCP for android emulator #87

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

HCP for android emulator #87

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34739e6
HCP for android emulator
RealHandy Oct 29, 2019
8c58b07
Extracted constants per @felipenevola request
RealHandy Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/android/AssetBundle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public String getRootUrlString() {
JSONObject runtimeConfig = getRuntimeConfig();
if (runtimeConfig != null) {
try {
rootUrlString = runtimeConfig.getString("ROOT_URL");
rootUrlString = runtimeConfig.getString("ROOT_URL").replace("localhost","10.0.2.2");
Copy link
Collaborator

@filipenevola filipenevola Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A few suggestions:
1 - Could you split this operation into two lines (not replacing directly after getString) to improve readability;
2 - Extract these string localhost and 10.0.2.2 for constants then we can have a readable name for both;
3 - I don't remember if we have the environment available in the Java part but it would be good to only apply this replace in development mode.

Copy link
Contributor Author

@RealHandy RealHandy Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, HCP doesn't work in Android emulator without this. That's the only purpose of it, is to make HCP work in Android emulator.

The proposed change is a straight steal from this existing meteor code, which is solving the same problem:

https://github.com/meteor/meteor/blob/3051150f2f5ae953f391802e73682fba613b3d46/packages/boilerplate-generator/template-web.cordova.js#L42-L50

I can split the lines for readability and extract the strings as constants, but I don't know what the test would be for whether it's dev or not. Is there a standard test for that? I'm pretty sure it already can only impact dev, because the server won't be localhost in production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm ok, if Meteor is already doing the same I believe this is safe. Please just update your code to have constants and let's move this forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool -- I extracted the constants and split the line into two.

} catch (JSONException e) {
Log.w(LOG_TAG, "Error reading ROOT_URL from runtime config", e);
}
Expand Down