Skip to content
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

feat: create local proxy library that is Go Mobile-friendly #64

Merged
merged 14 commits into from
Sep 7, 2023

Conversation

fortuna
Copy link
Contributor

@fortuna fortuna commented Sep 5, 2023

In this PR:

  • Add x/appproxy library that runs a local proxy given an address and config
  • Add x/examples/fetch-proxy that illustrates the pattern of running a local proxy and configuring a networking library.
  • Fix web proxy handler to support regular web proxy requests. The fetch-proxy wouldn't work for HTTP pages otherwise.
  • Add x/examples/mobileproxy, which illustrates how to build the local proxy library for mobile.

Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks, added some comments.

x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? It seems we don't have any actual code generating this folder, the only place we mentioned it is in the document. I feel it's OK to un-track it if it's only generated in a documentation, and we already mentioned how to clean it up in the doc.

x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/httpproxy/connect_handler.go Show resolved Hide resolved
Copy link
Contributor Author

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Another look?

x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/appproxy/appproxy.go Outdated Show resolved Hide resolved
x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/examples/mobileproxy/README.md Outdated Show resolved Hide resolved
x/httpproxy/connect_handler.go Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about moving this to x/appproxy and call it x/mobileproxy, so it's clear it's for Go Mobile?

x/appproxy shouldn't really be used by Go code, since it has a limited API that is not really optimal for Go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I updated the PR to have everything in x/mobileproxy. This way we can keep the library as clearly intended for Go Mobile.

@fortuna fortuna requested a review from jyyi1 September 6, 2023 16:57

### Build the iOS and Android libraries with [`gomobile bind`](https://pkg.go.dev/golang.org/x/mobile/cmd/gomobile#hdr-Build_a_library_for_Android_and_iOS)

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a TODO to publish these to Swift packages/maven?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Providing a wrapper library would be better than documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is definitely something that will help and we should consider. The downside is that it will require a releases process. Let's see how people end up using it and think about where we would put the binaries. To be defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's why I said we should at least track it with a TODO, or maybe an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I don't want to set the expectation that we will be providing the pre-packaged libraries. I want to see if what we have here will be good enough first, and we can consider the next step if people keep struggling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is every TODO or thing in the backlog something we're committing to definitely doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for not having a TODO for the release is that we are not currently intending to do it. Adding a TODO to a README file will set the wrong expectation. It's different from a code TODO.

We've been keeping the roadmap in the root README.

Note: Gomobile expects gobind to be in the PATH, that's why we need to prebuild it, and set up the PATH accordingly.

<details>
<summary>iOS generated Code</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unclear to me why this is in here. Won't this diverge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to make it clearer what the actual mobile interfaces look like, since that's what people will be using. It clarifies what the output is too.

It shouldn't diverge if we don't change the API.

Copy link
Contributor

@daniellacosse daniellacosse Sep 6, 2023

Choose a reason for hiding this comment

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

Sure, but why not commit the files and then just link to them? And generate them in pre-commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committing generated code is generally bad practice, as it diverges and it's hard to tell where the generated file came from. The releases, if we decide to do them, will include the binaries and header files.

The goal of having the code here is educational, so people know what the native library looks like.

Copy link
Contributor

@daniellacosse daniellacosse Sep 6, 2023

Choose a reason for hiding this comment

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

But you're committing generated code here? Why is it okay when you do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's just for illustration, it's not code to be used. People won't be copying and pasting it. They will run the commands in the README instead.

I've updated the text to say "Sample" code to make it clear that may not be the actual code to be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe adding "for reference", or "do not use in production" warning before the code snippet would make it more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment and a warning.

Note that there's no risk in accidentally using this in production, since you actually need the XCFramework/AAR with the binaries in it.

Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks, the code looks good, added a few nit-picking comments.

README.md Show resolved Hide resolved

### Build the iOS and Android libraries with [`gomobile bind`](https://pkg.go.dev/golang.org/x/mobile/cmd/gomobile#hdr-Build_a_library_for_Android_and_iOS)

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Providing a wrapper library would be better than documentation.

x/mobileproxy/mobileproxy.go Show resolved Hide resolved
@fortuna fortuna requested a review from jyyi1 September 6, 2023 19:13
README.md Outdated Show resolved Hide resolved
@fortuna fortuna merged commit a5564f7 into main Sep 7, 2023
6 checks passed
@fortuna fortuna deleted the fortuna-appproxy branch September 7, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants