payload: {"action":"created","issue":{"url":"https://api.github.com/repos/twitchtv/twirp/issues/179","repository_url":"https://api.github.com/repos/twitchtv/twirp","labels_url":"https://api.github.com/repos/twitchtv/twirp/issues/179/labels{/name}","comments_url":"https://api.github.com/repos/twitchtv/twirp/issues/179/comments","events_url":"https://api.github.com/repos/twitchtv/twirp/issues/179/events","html_url":"https://github.com/twitchtv/twirp/issues/179","id":455388424,"node_id":"MDU6SXNzdWU0NTUzODg0MjQ=","number":179,"title":"v6 removal of /twirp prefix makes upgrading hard","user":{"login":"spenczar","id":880413,"node_id":"MDQ6VXNlcjg4MDQxMw==","avatar_url":"https://avatars1.githubusercontent.com/u/880413?v=4","gravatar_id":"","url":"https://api.github.com/users/spenczar","html_url":"https://github.com/spenczar","followers_url":"https://api.github.com/users/spenczar/followers","following_url":"https://api.github.com/users/spenczar/following{/other_user}","gists_url":"https://api.github.com/users/spenczar/gists{/gist_id}","starred_url":"https://api.github.com/users/spenczar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spenczar/subscriptions","organizations_url":"https://api.github.com/users/spenczar/orgs","repos_url":"https://api.github.com/users/spenczar/repos","events_url":"https://api.github.com/users/spenczar/events{/privacy}","received_events_url":"https://api.github.com/users/spenczar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2019-06-12T19:37:25Z","updated_at":"2019-06-12T20:27:04Z","closed_at":null,"author_association":"MEMBER","body":"The current [v6_prerelease branch](https://github.com/twitchtv/twirp/tree/v6_prerelease) changes the URL scheme. It goes from `/twirp/<package>.<service>/<method>` to just `/<package>.<service>/<method>`.\r\n\r\nUnfortunately, this is difficult to upgrade into. Consider a service which has generated v5 code today, and which has a client which vendors that generated Go code. The service wants to upgrade to generating v6 code, and they currently register their Twirp service like this:\r\n\r\nIf they regenerate their code and don't tell the client to upgrade their vendored copy, then all client calls immediately start failing, since the client is hitting the old URL, which 404s. If the client upgrade first, same thing - all 404s. The client and server need to deploy an upgrade version simultaneously. That's unacceptably painful. We need something better.\r\n\r\nWhat are the options?\r\n\r\n1. **Document code changes the service owner can make**\r\n\r\nWe could tell service owners that they must run their service on two URL path prefixes if they are upgrading, at least until all their clients are updated. For example, they'd need to do this:\r\n```go\r\nmux := http.NewServeMux()\r\nmux.Handle(HaberdasherPathPrefix, svc)\r\nmux.Handle(\"/twirp\" + HaberdasherPathPrefix, http.StripPrefix(\"/twirp\", svc))\r\n```\r\n\r\nThis is not very pleasant. It's gross legacy code, and hard to know you need to do this; I don't like solutions that require manual action from our users. It's hard to know when you can remove this code, as well. And if users already have a mux, they could get confused. \r\n\r\nWe could make this more visible by renaming the PathPrefix constant. That way, code won't compile, and hopefully the user goes and looks up what's wrong and learns that they need to make a code change. But this is a bad option; upgrading should be painless.\r\n\r\n2. **Leave /twirp as a default prefix, but allow it to be customized**\r\n\r\nWe could change the branch to default to using the /twirp prefix. But if the user specifies, we could remove the prefix in generated code. This would make upgrading easy - the user would only run into trouble if they enabled a custom prefix, which is kind of their own fault. Most upgrades would be painless.\r\n\r\nBut I already rejected this approach in https://github.com/twitchtv/twirp/issues/48, really. My argument there was that non-customizability is a feature, not a bug. It makes third-party generators harder to get right, and complicates the spec a bit. That said, it's worth looking at how we'd do this.\r\n\r\nProtobuf files support options. We could have something like this:\r\n\r\n```protobuf\r\n// this path depends on your local machine :(\r\nimport \"path/to/twitchtv/twirp/genconfig.proto\" \r\n\r\nservice Haberdasher {\r\n  option (twitchtv.twirp.genconfig.prefix) = \"/whatever/you/want\"\r\n  rpc MakeHat(Size) returns (Hat)\r\n}\r\n```\r\n\r\nThe challenge is really that the option must be defined in some other `.proto` file which gets imported by the user. Twirp would have to distribute that `.proto` file somehow, and provide instructions on how to import it. This is tricky - protoc is not a friendly tool. But perhaps this is alright, since we hope that custom prefixes are the exceptional case, not the common one, so it's alright for them to be a little difficult.\r\n\r\n3. **Go back to requiring /twirp prefix**\r\n\r\nFinally, we could revert this change entirely. This is appealing to me - it keeps Twirp simple and the upgrade is _really_ clean. I like that no third-party generators become incompatible, since the protocol remains the same.\r\n\r\nBut there are a few people who are using the prerelease branch today because they hate that prefix. I'd like to hear from them on their needs, once again. We kind of touched on them in #55, but that covered other topics, and I don't think we have good written reasons that removing the /twirp prefix _specifically_ is worth the cost. I'd like to hear those arguments.\r\n\r\n---\r\n\r\nI lean towards option 3, in absence of clear reasons the /twirp prefix needs to be removed."},"comment":{"url":"https://api.github.com/repos/twitchtv/twirp/issues/comments/501442391","html_url":"https://github.com/twitchtv/twirp/issues/179#issuecomment-501442391","issue_url":"https://api.github.com/repos/twitchtv/twirp/issues/179","id":501442391,"node_id":"MDEyOklzc3VlQ29tbWVudDUwMTQ0MjM5MQ==","user":{"login":"marwan-at-work","id":16294261,"node_id":"MDQ6VXNlcjE2Mjk0MjYx","avatar_url":"https://avatars0.githubusercontent.com/u/16294261?v=4","gravatar_id":"","url":"https://api.github.com/users/marwan-at-work","html_url":"https://github.com/marwan-at-work","followers_url":"https://api.github.com/users/marwan-at-work/followers","following_url":"https://api.github.com/users/marwan-at-work/following{/other_user}","gists_url":"https://api.github.com/users/marwan-at-work/gists{/gist_id}","starred_url":"https://api.github.com/users/marwan-at-work/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/marwan-at-work/subscriptions","organizations_url":"https://api.github.com/users/marwan-at-work/orgs","repos_url":"https://api.github.com/users/marwan-at-work/repos","events_url":"https://api.github.com/users/marwan-at-work/events{/privacy}","received_events_url":"https://api.github.com/users/marwan-at-work/received_events","type":"User","site_admin":false},"created_at":"2019-06-12T20:27:04Z","updated_at":"2019-06-12T20:27:04Z","author_association":"NONE","body":"I think keeping things simple and stable is a better feature than removing `/twirp`. Though I'd prefer the `/twirp` prefix to be removed to hide a server's internals. \r\n\r\nSo my vote is either #2 or #3 \r\n\r\nFor #3, I think we can maybe just document how to hackishly remove that prefix for any user who wants to do it like @rynop suggested. For example, by adding a Go middleware that just prepends the `/twirp` prefix. \r\n\r\n"}}
other: {"actor":{"display_login":"marwan-at-work"}}