derbyjs / racer

Realtime model synchronization engine for Node.js
1.18k stars 116 forks source link

Fix breaking change in types introduced by `ValueCallback` #308

Closed deongroenewald closed 2 months ago

deongroenewald commented 2 months ago

The introduction of the ValueCallback type in #305 inadvertently introduced a breaking change in the types.

It adds an additional constraint that the passed-in callback function must also expect null as a possible value for the err parameter in addition to Error and undefined whereas the ErrorCallback type does not.

I don't believe null is a possible return value for the err parameter based on the previous code so suggest removing the null from the union type for err in ValueCallback so that it matches ErrorCallback

ericyhwang commented 2 months ago

Thanks for the report and PR.

For this specific case of add, it happens that the current implementation will not pass null for the error, so I'll go ahead and get this in to resolve the unintentional breaking change.

However, in the general case on Node-style callbacks with results, null can commonly be passed for the error.


On Node-style callbacks:

For a function implementation, the standard way of calling the callback with a result does include null:

// with result and no error
callback(null, result);
// with no error
callback();

Node's core library type definitions have numerous examples of this - they actually don't include undefined: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d8e5f63a03750e5711e42cdc7bff942fc9e31e93/types/node/fs.d.ts#L2733-L2741

ShareDB's query code can invoke the callback with either approach in the example above: https://github.com/share/sharedb/blob/f4effa3a73c840cfeff8ed59371fda16516d7ad9/lib/client/query.js#L155

Now, for op submissions, ShareDB doesn't provide a result, so the error will only be undefined or an Error. The new Racer code for the model.add callback getting the id result just passes through the error or lack thereof, so it happens that the error will never be null.