Normalize PublicClient with AuthenticatedClient

[rmm5t]

What does it do?

• Deprecates productID argument in PublicClient constructor
• Requires productID as first argument to getProductOrderBook() and deprecates old usage signature
• Removes redundant AuthenticatedClient#getProductOrderBook() override
• Simplifies OrderbookSync#loadOrderbook() because both clients now adhere to the same method signature
• Requires productID as first argument to getProductTicker() and deprecates old usage signature
• Requires productID as first argument to getProductTrades() and deprecates old usage signature
• Requires productID as first argument to getProductTradeStream() and deprecates old usage signature
• Requires productID as first argument to getProductHistoricRates() and deprecates old usage signature
• Requires productID as first argument to getProduct24HrStats() and deprecates old usage signature
• Consolidates deprecation & argument normalization logic in PublicClient to later simplify the eventual transition of outright removing the (now unnecessary) PublicClient#productID property.
• Updates the TypeScript definitions to match
• Updates the README documentation to match
• Includes several tests that were missing
• Includes backwards compatibility tests for all deprecated method signatures

What else do you need to know?

• Any use of a deprecated method signature will emit a warning
• The general idea is that PublicClient should no longer maintain state about a particular product ID.
• Any method that can be called on a PublicClient instance can now also be called on an AuthenticatedClient instance. In other words, the classes now correctly adhere to the specification in the README:

  "AuthenticatedClient inherits all of the API methods from PublicClient, so if you're hitting both public and private API endpoints you only need to create a single client."

Related Issues and PRs

• Fixes Authenticated getProductOrderBook overridden #46
• Fixes AuthenticatedClient doesn't allow specification of a productID for inherited PublicClient methods #117
• Fixes Authed client can't get product ticker #158
• Fixes getProductTrades in AuthClient #165
• Closes Add productId to authenticated clients & some cleanup #83 (this PR supersedes)
• Closes Added productID to authClient, updated index.d.ts, updated readme #131 (this PR supersedes)

Communication

• Because this is a fairly large PR, I recommend code-reviewing the changes for each commit individually instead of reviewing the entire PR at once. It's likely easier to digest that way.

• I think it would be prudent to make an interim v0.x release with these changes, thereby documenting and committing to all the deprecations. That would allow for a full cleanup during an eventual v1.0 release where the old method signatures and deprecations are removed, and the new method signature win-out.

/cc @fb55

[fb55]

Awesome PR, thanks a lot!

Regarding npm releases: How do you feel about doing 1.0 beta releases?

[rmm5t]

Regarding npm releases: How do you feel about doing 1.0 beta releases?

@fb55 For this project (and almost all open source projects), I'm always going to advocate for more frequent minor releases vs one large major release drop. Here, this means at least an official v0.5 (back) release and a v0.6 release prior to going to v1.0.

Reasons:

1. When true deprecations are introduced to a codebase (as they are in this PR), it's imperative that those deprecations get published out in a minor release as soon as possible. It's best to have the community run into them early and fix the warnings before the deprecation warnings are eventually removed.
2. When a major version bump happens, the deprecations should be removed, and the codebase made as clean as possible (avoiding the old deprecation conditional checks).
3. Almost no one tests beta or rc1 (release candidate) releases. They are only valuable for really large progjects (operating systems, web frameworks, languages) and/or really popular projects that have large upgrade paths and potentially untested territory. This project is much too small to glean any kind of benefit from a beta release.
4. True and more-frequent semver'd releases allow developers to more easily peg themselves to an older version of a library if they don't yet have the time to perform a major upgrade. It not only improves stability amongst the community, but it also instills confidence in the open source library itself and the maintainers themselves.

In summary, an official beta release will yield almost no benefit to this project. Frequent minor releases will get used and allow us to iterate more quickly and with more confidence.

---

Also, with all that said, even with this PR's normalization of PublicClient and AuthenticationClient, I think there is still room to improve the method signatures through the library.

1. Positional arguments for a library like this that wraps another REST API makes little sense. I think we should move this library towards keyword arguments as much as possible.
   1. Keyword arguments better document themselves.
   2. Keyword arguments allow for easier forwards compatibility changes. We can add features without affecting existing codebases.
2. The constructor for PublicClient and AuthenticationClient still don't match. This is a code-smell from an objected-oriented perspective. Both should use keyword arguments. This would allow you to instantiate either client interchangeably based on a variable pointing at the class as long as you expect to use the parent interface. e.g. This would further cleanup the OrderbookSync internal implementation.

Another concrete example:

// Existing API signature (positional arguments make things unnecessarily messy)
const ob = new OrderBookSync(['BTC-USD'],  'https://api.gdax.com', 'wss://ws-feed.gdax.com', { key, secret, passphrase })

//  Future potential API signature (far superior with keyword arguments)
const productIDs = ['BTC-USD'];
const auth = { key, secret, passphrase };
const ob = new OrderBookSync({ productIDs, auth }); // order doesn't matter, no duplicate defaults

Perhaps, keyword arguments need to wait until a v2.0 release, BUT these are the kinds of considerations that make more-frequent releases more important, because they allow the community to keep up with rapid development and change. Allow for change and prepare for change, but don't hold too tightly to backwards compatibility forever.

Forwards compatibility is the more interesting and beneficial design paradigm.

[tkriplean]

Thanks @rmm5t!

[vansergen]

It seems that #362 solves the issues that @rmm5t mentioned

1. Positional arguments for a library like this that wraps another REST API makes little sense. I think we should move this library towards keyword arguments as much as possible.

   1. Keyword arguments better document themselves.
   2. Keyword arguments allow for easier forwards compatibility changes. We can add features without affecting existing codebases.

2. The constructor for PublicClient and AuthenticationClient still don't match. This is a code-smell from an objected-oriented perspective. Both should use keyword arguments. This would allow you to instantiate either client interchangeably based on a variable pointing at the class as long as you expect to use the parent interface. e.g. This would further cleanup the OrderbookSync internal implementation.