-
Notifications
You must be signed in to change notification settings - Fork 55
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
connectivity test app: android #74
Conversation
daniellacosse
commented
Sep 14, 2023
•
edited
Loading
edited
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.
This is very exciting. I love the screenshot. Here are some minor things
x/examples/outline-connectivity-app/app_mobile/capacitor.config.ts
Outdated
Show resolved
Hide resolved
androidTestImplementation "androidx.test.espresso:espresso-core:$androidxEspressoCoreVersion" | ||
implementation project(":capacitor-cordova-android-plugins") | ||
implementation "org.jetbrains.kotlinx:kotlinx-serialization-json:1.6.0" | ||
implementation files('../../../shared_backend/output/SharedBackend.aar') |
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.
You might want to add a comment calling out that you had to add this line. I imagine all the rest was auto generated?
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.
I guess? Any dependencies we add we'll need to add to this file, it's standard android development.
@@ -17,5 +17,8 @@ import { defineConfig } from "vite"; | |||
export default defineConfig({ | |||
build: { | |||
outDir: './output/frontend', | |||
}, | |||
server: { |
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.
How is this port used? Do you need to hardcode the port? What if there's a service there, will the app not work?
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.
The server is for local development only - in production the web assets are bundled directly into the app.
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.
As per above, bind it to localhost here too. The default is probably all IPs.
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.
default is localhost: https://vitejs.dev/config/server-options.html#server-host
appName: "@outline_sdk_connectivity_demo/app_mobile", | ||
webDir: "output/frontend", | ||
server: { | ||
url: "http://10.0.2.2:3000" |
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.
Why does it need a local network address?
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.
The server is for local development only - in production the web assets are bundled directly into the app.
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.
Let's bind to localhost, so it doesn't risk external access. That's a security vulnerability for the developer (the dev app may have a vulnerability that allows for remote code execution, for example).
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.
The android emulator doesn't have access to localhost: this is just a setting that's copied into the android app to know to open 10.0.2.2 in the web view, not a configuration for the server itself
Can you add the screenshot to the app README at https://github.com/Jigsaw-Code/outline-sdk/tree/main/x/examples/outline-connectivity-app? That will make the README way more compelling. It may be helpful to update #42 as well (or post a new entry). |
I need to add some android-specific styles, and then yeah I'll retake the photo with that added. |