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

Fix breaking change in types introduced by ValueCallback #308

Merged
merged 1 commit into from
May 27, 2024

Conversation

deongroenewald
Copy link
Contributor

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
Copy link
Contributor

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.

@ericyhwang ericyhwang merged commit 4d153f3 into derbyjs:master May 27, 2024
8 checks passed
@deongroenewald deongroenewald deleted the fix/value-callback-type branch May 27, 2024 20:16
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.

2 participants