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

Rename connection to tunnel #863

Merged
merged 4 commits into from Oct 2, 2020
Merged

Rename connection to tunnel #863

merged 4 commits into from Oct 2, 2020

Conversation

alalamav
Copy link
Contributor

  • Renames connection to tunnel in cordova.plugin.outline and native platform implementations.
  • Minor documentation updates.
  • I have left TODOs to rename storage keys when we perform a data migration (i.e. update server model schema), in order to make this change only a code refactor.

@alalamav alalamav self-assigned this Sep 28, 2020
Copy link
Contributor

@JonathanDCohen JonathanDCohen left a comment

Choose a reason for hiding this comment

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

Can you leave some thoughts as to why Tunnel makes more sense than Connection? Because we can multiplex connections over a single tunnel?

@@ -298,26 +300,26 @@ async function startVpn(
throw new Error('already connected');
}

currentConnection = new ConnectionManager(config, isAutoConnect);
currentConnection = new TunnelManager(config, isAutoConnect);
Copy link
Contributor

Choose a reason for hiding this comment

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

currentTunnel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, done in 07fcf56.


## JavaScript API

```ts

// Represents a Shadowsocks server configuration.
interface ShadowsocksServerConfig {
interface ServerConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to change this name as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this interface was always ServerConfig. The documentation was not in sync.

@alalamav
Copy link
Contributor Author

alalamav commented Oct 1, 2020

Can you leave some thoughts as to why Tunnel makes more sense than Connection? Because we can multiplex connections over a single tunnel?

That's right. I think the abstraction fits better with networking terminology (i.e. "apps establish TCP connections over a VPN tunnel", TUN device) and native platform concepts (i.e. Apple's PacketTunnelProvider, our own Android VpnTunnelService).

Thank you for the review :)

@alalamav alalamav merged commit 38cc535 into master Oct 2, 2020
@alalamav alalamav deleted the alalama-tunnel branch October 2, 2020 20:40
github-actions bot added a commit to Wolf1329/outline-client that referenced this pull request Oct 3, 2020
@alalamav alalamav mentioned this pull request Nov 17, 2020
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.

None yet

2 participants