RFE: Make pusher/puller interface lower level

[aboodman]

See discussion: https://discord.com/channels/830183651022471199/830183651022471202/879401020923478016

[arv]

Needs some API design.

Pull:

We currently have:

function callPuller(
  puller: Puller,
  url: string,
  body: PullRequest,
  auth: string,
  requestID: string,
): Promise<PullerResult>;

type PullerResult = {
  response?: PullResponse;
  httpRequestInfo: HTTPRequestInfo;
};

type HTTPRequestInfo = {
  httpStatusCode: number;
  errorMessage: string;
};

export type Puller = (request: Request) => Promise<PullerResult>;

export type PullRequest = {
  clientID: string;
  cookie: ReadonlyJSONValue;
  lastMutationID: number;
  pullVersion: number;
  // schema_version can optionally be used by the customer's app
  // to indicate to the data layer what format of Client View the
  // app understands.
  schemaVersion: string;
};

push:

We currently have:

function callPusher(
  pusher: Pusher,
  url: string,
  body: PushRequest,
  auth: string,
  requestID: string,
): Promise<HTTPRequestInfo>;

type Pusher = (request: Request) => Promise<HTTPRequestInfo>;

type PushRequest = {
  clientID: string;
  mutations: Mutation[];
  pushVersion: number;
  schemaVersion: string;
};

Strawman:

Use callPuller/callPusher as the interface (minus passing the puller/pusher).

We need a new name for these options though...

[aboodman]

I think that we should not pass auth either. Some users who wanted this suggested auth as a reason to do their own puller/pusher implementations and that makes sense to me.

[aboodman]

Also no reason to pass URL or to need a result from pull. So I would suggest:

// Developer can optionally string `requestID` through their implementation for logging purposes.
// Developer should `throw` to indicate any type of failure to Replicache. Replicache will backoff exponentially.
handlePush(requestID: string, request: PushRequest) => Promise<void>;
handlePull(requestID: string, request: PullRequest) => Promise<PullResponse>;

[paulshen]

looks good! some reactions

• Could requestID be part of PushRequest and PullRequest?
• I wonder if PullerResult would benefit with something like a tagged union.

type PullResult =
| { result: 'ok', response: PullResponse }
| { result: 'error', errorMessage: string };

In current form, what happens if status code is 200 but you have an error code or are missing a response? Or if status code is 500 but you have a response?

[aboodman]

Could requestID be part of PushRequest and PullRequest

Sure. Fair point!

I wonder if PullerResult would benefit with something like a tagged union.
what happens if status code is 200 but you have an error code or are missing a response

The current internal API is somewhat crufty and out of date. The HTTPRequestInfo bit is not used at all, and not needed.

At a basic level, Replicache does not care at all what the response to push is. It's a one way message. Replicache it built on the premise that it might not get the response to push -- as all distributed systems must be.

Similarly Replicache does not care what the response to pull is, so long as it is a valid PullResponse. Any response other than a valid PullResponse is an error to Replicache.

The only way errors or other responses matter in these two requests is for developer debugging. Since the contents of HTTP responses are displayed in the network pane and errors are color-coded, we encourage developers to use 4xx, 5xx appropriately and return useful error responses. But that's entirely for their own benefit, it doesn't matter to Replicache.

[aboodman]

This is hard to do as a non-breaking change for a patch release because there's a lot of cruft in the existing push/pull support, but here's my proposal for doing this as a breaking change (maybe for 9.0).

Big picture:

1. Get rid of pushURL/pullURL
2. Get rid of all the auth goop (authURL, getAuth, etc)
3. Replace pusher/puller with new push and pull callbacks (see below)

Do the above as a breaking change (e.g., for 9.0) so that we don't have to maintain all the old code paths. They have gotten pretty intricate and it would be a lot of work to maintain them.

type ReplicacheOptions = {
  ...
  push?: Push;
  pull?: Pull;
  ...
};

/**
 * Implements the push operation for Replicache sync.
 * 
 * In case of error, either return false or throw. Thrown errors
 * will be re-thrown and bubble up to top of JS context. In either
 * case, Replicache will exponentially backoff using the parameters
 * from ReplicacheOptions.requestOptions.
 */
type Push = (req: PushRequest) => Promise<boolean>;

/**
 * Implements the pull operation for Replicache sync.
 * 
 * In case of error, either return null or throw. Thrown errors
 * will be re-thrown and bubble up to top of JS context. In either
 * case, Replicache will exponentially backoff using the parameters
 * from ReplicacheOptions.requestOptions.
 */
type Pull = (req: PullRequest) => Promise<PullResponse|null>;

type PushRequest = TodaysPushRequest & { requestID: string; };
type PullRequest = TodaysPullRequest & { requestID: string; }; 

So to transition after this change, a user who uses pushURL today would do:

new Replicache({
- pushURL: myPushURL,
+ push: (req) => {
    const res = await fetch(myPushURL, "POST", { body: req });
    return res.status === 200;
  }
}

... similar changes for users who use pullURL/pusher/puller.

[aboodman]

@arv @grgbkr @phritz FYI

[arv]

Looks good to me but I think we should maybe have some default implementations; defaultHTTPPush(url)/defaultHTTPPull('url').

[aboodman]

@arv bumping this it comes up all the time. So annoying.

[aboodman]

On second thought we should probably wait until we integrate Reflect as that's going to affect this interface most likely.

[arv]

On second thought we should probably wait until we integrate Reflect as that's going to affect this interface most likely.

That makes sense. Let's hold off on this until then.

[aboodman]

This was implemented in Replicache 13.