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

Define, document, and release a stable (v1) version. #46

Closed
creachadair opened this issue Apr 19, 2021 · 30 comments
Closed

Define, document, and release a stable (v1) version. #46

creachadair opened this issue Apr 19, 2021 · 30 comments
Assignees

Comments

@creachadair
Copy link
Owner

creachadair commented Apr 19, 2021

This module is currently versioned for development (v0).

Where practical, I try to avoid breaking API changes. However, when I do need to change the API I've adopted the convention of incrementing the minor version number. For other changes I will generally only increment the patch number. Hence, v0.11.1 to v0.11.2 should be a "safe" (non-breaking) upgrade, whereas v0.12.1 to v0.13.0 includes visible API effects that may require client code to be updated.

This issue was created to track the definition and release of a stable (v1) version. Broadly: I would like to trim down the API as much as possible for such a release.

Summary of Changes

package jrpc2

package jhttp

package channel

  • Remove the separate Sender and Receiver interfaces (65bf9ad)
  • Remove the unused WithTrigger wrapper channel (a5d1a82)

package handler

package server

Other

@creachadair creachadair self-assigned this Apr 19, 2021
@creachadair
Copy link
Owner Author

creachadair commented May 4, 2021

Candidates for removal:

  1. handler.NewService. Apart from the convenience of extracting the names, this has no real advantage over explicitly constructing the map. Moreover, it cannot handle unexported methods, and the names it assigns are whatever Go uses, not necessarily what a service definition requires. Better to let the caller be explicit. ✅ 0ee4b78
  2. channel.Varint. Convert to example code. ✅ 2534128

@creachadair
Copy link
Owner Author

creachadair commented May 4, 2021

I would like to simplify the server package somewhat, and improve the naming. The terms "service", "simple", and "static" are used confusingly, and I think should be cut down.

My current thinking is to consolidate "simple" and "static" into a single concept: A "static" service implementation is one without a meaningful constructor. Instead of a separate type for single-throw services, the package can have a top-level Run function that does the work.

4bb7b22

@creachadair
Copy link
Owner Author

creachadair commented May 4, 2021

Possible server simplification: Re-combine the Wait and WaitStatus methods, and give the error reported by Wait a documented concrete type. Now, Wait reports the underlying error; with this change it would always return a status value.

❌ Decided not to do this; conflating these two doesn't save much and the semantics are different enough to be worth keeping separate.

@creachadair
Copy link
Owner Author

creachadair commented May 5, 2021

Possible error simplification: Export most or all of the fields of the *Error type, and drop the methods. Support for marshaling and unmarshling JSON is still necessary, but making the type usable directly is more idiomatic.

b11415d

@creachadair
Copy link
Owner Author

creachadair commented May 5, 2021

Possible server simplification: Now that the server is exposed to handlers via the context, there is no longer any need for separate top-level PushNotify, PushCallback, or ServerMetrics functions.

To get rid of ServerMetrics would require adding a method to expose it, though.

d4361e1

creachadair added a commit that referenced this issue May 5, 2021
Simplify fixtures, remove unneeded method.

#46 (comment)
creachadair added a commit that referenced this issue May 6, 2021
- Rework NewStatic as "Static", which returns a plain constructor.
- Remove the "Simple" type and move its Run method to a top-level function.
- Update usage examples.

#46 (comment)
creachadair added a commit that referenced this issue May 9, 2021
This constructor is not carrying its weight in API surface.  It cannot handle
unexported methods, and the names it extracts are based on Go-specific lexical
rules rather than whatever a service may require.

Simplify test fixtures and remove unnecessary support code.

See issue #46.
creachadair added a commit that referenced this issue May 9, 2021
This constructor is not carrying its weight in API surface.  It cannot handle
unexported methods, and the names it extracts are based on Go-specific lexical
rules rather than whatever a service may require.

Simplify test fixtures and remove unnecessary support code.

See issue #46.
creachadair added a commit that referenced this issue May 9, 2021
- Rework NewStatic as "Static", which returns a plain constructor.
- Remove the "Simple" type and move its Run method to a top-level function.
- Update usage examples.

#46 (comment)
creachadair added a commit that referenced this issue May 9, 2021
- Rework NewStatic as "Static", which returns a plain constructor.
- Remove the "Simple" type and move its Run method to a top-level function.
- Update usage examples.

#46 (comment)
creachadair added a commit that referenced this issue May 10, 2021
Now that the *Server can be recovered directly from the handler context, there
is no longer a need for separate top-level helper functions in the package.
Remove these to simplify the package API.

Note that this is a breaking change. To update existing use, replace:

  jrpc2.PushNotify(ctx, m, p) ⇒
  jrpc2.ServerFromContext(ctx).Notify(ctx, m, p)

  jrpc2.PushCall(ctx, m, p) ⇒
  jrpc2.ServerFromContext(ctx).Callback(ctx, m, p)

  jrpc2.CancelRequest(ctx, id) ⇒
  jrpc2.ServerFromContext(ctx).CancelRequest(id)

  jrpc2.ServerMetrics(ctx) ⇒
  jrpc2.ServerFromContext(ctx).Metrics()

See issue #46.
creachadair added a commit that referenced this issue May 10, 2021
- Rework NewStatic as "Static", which returns a plain constructor.
- Remove the "Simple" type and move its Run method to a top-level function.
- Update usage examples.

#46 (comment)
creachadair added a commit that referenced this issue Nov 26, 2021
Further API cleanup related to #46.

- 63a5fca Remove the client and server "AllowV1" settings. (#61)
- 7ab9b38 Remove the CheckRequest server option. (#62)
- 643e3dc Remove the "UsesContext" field from ServerInfo.
@creachadair
Copy link
Owner Author

Remove the parameter-wrapping context plumbing hooks. This was meant as a transparent way to encode context settings, but it was a lot of overhead for relatively little use.

#82

@creachadair
Copy link
Owner Author

creachadair commented Oct 30, 2022

Replace the custom metrics collector plumbing with the standard expvar package. This will make it easier to export metrics to more conventional collectors like Prometheus.

#89

@creachadair
Copy link
Owner Author

creachadair commented Dec 10, 2022

Simplify the Handler type to be a plain function instead of an interface.

#90

@creachadair
Copy link
Owner Author

creachadair commented Mar 14, 2023

Rip out the registration mechanism for error code values. In practice any package that wants to define custom codes has to keep track of them anyway, so that the client can coordinate with the server. The additional hook for cosmetics in the default error string is just not worthwhile.

#91

@creachadair
Copy link
Owner Author

creachadair commented Mar 14, 2023

Look for usage of the server.Run function and see if we can get rid of it or adapt the use.

#92

creachadair added a commit that referenced this issue Mar 14, 2023
In practice any package that wants to define custom codes has to keep track of
them anyway, so that the client can coordinate with the server. The additional
hook for cosmetics in the default error string is just not worthwhile.

Updates #46.
creachadair added a commit that referenced this issue Mar 14, 2023
In practice any package that wants to define custom codes has to keep track of
them anyway, so that the client can coordinate with the server. The additional
hook for cosmetics in the default error string is just not worthwhile.

Updates #46.
creachadair added a commit that referenced this issue Mar 14, 2023
This has no meaningful use to justify its existence in the API.

Updates #46.
creachadair added a commit that referenced this issue Mar 14, 2023
This has no meaningful use to justify its existence in the API.

Updates #46.
creachadair added a commit that referenced this issue Mar 14, 2023
Another step toward #46.

Maintenance:
- Update module dependencies.
- handler: fix an outdated comment
- Remove the custom queue implementation. (#93)

Breaking changes:
- code: remove the code registration hooks (#91)
- server: remove the Run helper (#92)

Although these changes are breaking, I surveyed existing usage visible from the
search engine, and did not find anything likely to care.
creachadair added a commit that referenced this issue Mar 15, 2023
- Remove the RPCServerInfo client helper, it was unused outside tests.
- Remove the methodFunc wrapper type, it is no longer necessary.
- Fix up tests.

Updates #46.
creachadair added a commit that referenced this issue Mar 15, 2023
- Remove the RPCServerInfo client helper, it was unused outside tests.
- Remove the methodFunc wrapper type, it is no longer necessary.
- Collapse the now-vestigial special.go back into server.go.
- Fix up tests.

Updates #46.
@creachadair
Copy link
Owner Author

creachadair commented Mar 16, 2023

Convert handler.Func into a plain type alias and remove the repetition of the type signature in the Check plumbing.

e1278b5

creachadair added a commit that referenced this issue Mar 19, 2023
After removing the registration hooks in #91, what is left of the code package
is too small to deserve its own identity. Collapse the package into the root
jrpc2 package.

- Rename the code.FromError helper to jrpc2.ErrorCode.
- Update documentation.

Updates #46.
creachadair added a commit that referenced this issue Mar 19, 2023
After removing the registration hooks in #91, what is left of the code package
is too small to deserve its own identity. Collapse the package into the root
jrpc2 package.

- Rename the code.FromError helper to jrpc2.ErrorCode.
- Update documentation.

Updates #46.
@creachadair
Copy link
Owner Author

creachadair commented Mar 19, 2023

I think the package has reached a stable enough API that it's time to cut a v1. The remaining tasks will be:

  • Do a thorough audit of the package, type, and function docs (Update doc comments throughout the module. #98).
  • Update the README to reflect the stable API and remove the old language about pre-v1 tags (3540e6c).
  • Update and simplify the playground example linked from the README.
  • Survey existing use to make sure I haven't missed something important.

I haven't decided whether to bother with a separate release branch; I think probably not. If a v2 release becomes desirable I'll clone the code into a submodule, it's not that big a project.

creachadair added a commit that referenced this issue Mar 20, 2023
creachadair added a commit that referenced this issue Mar 20, 2023
creachadair added a commit that referenced this issue Mar 20, 2023
There is no longer any reason to have a distinct type for this, since the
interface was removed and jrpc2.Handler was converted to a type alias.  Clean
up all the uses of handler.Func and use jrpc2.Handler instead.

Updates #46.
@radeksimko
Copy link
Contributor

I'm not sure if it needs to change the API in any way to account for this - probably not necessarily backwards incompatible way - but unless you're already keeping eye, I'd recommend checking out the proposal for structured logging which was recently accepted: golang/go#56345

@creachadair
Copy link
Owner Author

creachadair commented Mar 20, 2023

Thanks for flagging that. I don't think the server and client debug logs would benefit much from adding structure, but if an application wants to use structured logs in handlers, it would be easy to plumb through a logger via the NewContext server option and handlers could use it, e.g.,

type slogKey struct{}

func WithLogger(ctx context.Context, s *slog.Logger) context.Context {
   return context.WithValue(ctx, slogKey{}, s)
}

func Logger(ctx context.Context) *slog.Logger {
   if s := ctx.Value(slogKey{}); s != nil {
      return s.(*slog.Logger)
   }
   return slog.Default()
}

opts := &jrpc2.ServerOptions{
  NewContext: func() context.Context {
    return WithLogger(context.Background(), myLogger)
  },
}

func Handle(ctx context.Context, req *jrpc2.Request) (any, error) {
   Logger(ctx).Info("hello from handler", "method", req.Method())
   return "ok", nil
}

or words to that effect.

I'm a little disappointed that this API got approved as-written, but I don't see any obvious compatibility concerns. If we later need to bolt structured logging on to the existing debug facility, it wouldn't be too bad to add another hook to the options.

@creachadair
Copy link
Owner Author

v1.0.0 is now tagged and available.

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

No branches or pull requests

3 participants