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

WIP: Version 6 #112

Closed
wants to merge 7 commits into from
Closed

WIP: Version 6 #112

wants to merge 7 commits into from

Conversation

spenczar
Copy link
Contributor

@spenczar spenczar commented Jun 6, 2018

Issue #, if available:
#3
#55
#70
#80

Description of changes:

This PR holds all the v6 changes, so they can be merged into master at once. While working on these features, they will be merged into the v6_prerelease branch to make them available for adventurous users.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@spenczar spenczar added this to the v6 milestone Jun 6, 2018
@jtvjorge
Copy link
Contributor

jtvjorge commented Jan 4, 2019

For anyone still interested, this is on hold until at least March of 2019. Thanks for your interest!

@rmasp
Copy link

rmasp commented Mar 3, 2019

For anyone still interested, this is on hold until at least March of 2019. Thanks for your interest!

Interested. And it's March. Questions/comments:

  1. There's been no updates for about 6 months. Is this release still happening?
  2. I only want the routing changes (wish that was the original protocol), not any streaming stuff. Why delay and complicate v6 with more moving parts?
  3. git checkout master && git merge v6_prerelease looks pretty clean and simple. How far out on a limb would I be if I fork and track my own until this gets done?

Great "batteries included" stuff here!

@fi0
Copy link

fi0 commented Mar 10, 2019

I wish the custom prefix routing #55 could be added in v5, instead of waiting for v6. Or streaming stuff can be a v7?

@spenczar
Copy link
Contributor Author

Yes, as I stated in #70, I think streaming might be the wrong next step for Twirp. That would simplify this release considerably, changing it down to just being about paths.

How far out on a limb would I be if I fork and track my own until this gets done?

@rmasp, I'm not sure exactly. The current branch just removes the /twirp prefix, but I think I might want to do something a little more sophisticated to make the migration process easier. That might come with a new API, or something - not exactly sure, but I'm back to thinking about it much more actively.

Thanks for waiting, and sorry about the long delay on this. The reason for the delay is mostly that Twirp works pretty well today, and these routing changes will take some careful work to make the transition smooth. I think they're basically just cosmetic, too.

@rmasp
Copy link

rmasp commented Mar 11, 2019

The current branch just removes the /twirp prefix, but I think I might want to do something a little more sophisticated to make the migration process easier

And that's all I want/need. Again, for those of us exposing our Twirp APIs to 3rd parties, the /twirp is an issue. However, I branched master, merged in v6_prerelease and everything looks good. And since I'm building anew, migration is a non-issue for me.

That said, why not make the lead path part configurable at protoc code-gen time, with nil or empty string leaving that path piece out entirely? Default would be /twirp, so legacy code would not be affected?

@spenczar
Copy link
Contributor Author

Right, I'd just like to avoid configuration at code-gen time if possible. Code generators are often buried in tool chains, so passing through options to them can be hard for people with any sophistication in their builds. An option in the .proto file might work, but that would require providing an importable definition, which adds its own headaches... maybe worth it, but it's a challenge.

@rmasp
Copy link

rmasp commented Mar 11, 2019

Seems like the approach I like puts the onus of toolchain config on those if us who don't like the default. It's a no-op for anyone happy with the /twirp default. Just sayin'.

@spenczar
Copy link
Contributor Author

Yep, but if I can avoid adding that complexity for anyone, I'd prefer that. Good point though, you've moved my thinking on this a bit.

@rmasp
Copy link

rmasp commented Mar 11, 2019

Thanks for listening! Fwiw, I'm using twirp in a pipeline with gqlgen, which has a bunch of code gen configurability in a yml: https://gqlgen.com/config/

Imho as twirp sees wider adoption you'll get requests for a more configurable settings. Look at Django, Rails, ... I know it's a fine line between simplicity and configurability. Way to many tools (esp in the golang space) have more dials and buttons than Spock's science station. Have to design for the "just right porridge" and common case defaults.

@rynop rynop mentioned this pull request Apr 11, 2019
@Bankq
Copy link

Bankq commented May 1, 2019

Shall we merge newer releases back to v6_prerelease branch in the meantime?

edit: I took the liberty created: #173

@rmasp
Copy link

rmasp commented Jul 12, 2019

Fwiw, next week I'm moving my servers (and clients) to the v6_prerelease branch because of product requirements that include no /twirp in path. I'd really like to see v6 become mainline.

@sylvain101010
Copy link

Hi @spenczar ,
Any news regarding a merge date ?

Also, thank you, twrip is awesome!

@marioizquierdo marioizquierdo removed this from the v6 milestone Jun 18, 2020
@marioizquierdo
Copy link
Contributor

Sorry, this PR is not going to be merged. The protocol v6 proposal was closed because both issues included in the release could not reach consensus (streaming: #3, and updating the path: #55).

This doesn't mean that the features will never be implemented in some form, this only means that they don't depend on each other for v6.

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.

7 participants