Resolve addPromised with new doc id

[craigbeck]

Pass id of newly created doc in callback form add and resolve with id for addPromised

Typescript definition already indicated resolving with string but wasn't.

[adin-ttc]

looks good but typescript may complain about undefined return typed as a string.

[adin-ttc]

return type 'undefined' is not assignable to type 'string'. consider adding to your interface : string | undefined

[craigbeck]

@ericyhwang what do you think of throwing an error here? Optional callback isn't great for the error handling case as this is really argument validation (so before any "work" that would result in callback call) but I'm unclear as to impacts or likelihood of this case. No id to return, and don't like the optional string return for downstream implications.

[craigbeck]

FWIW Typescipt is okay with this (incompletely typed), and as this is what already exists in current code, It's what it will stay with for now.

Does highlight another point though... our type sigs allow value: any and not constrained to object

[ericyhwang]

Throwing an error seems reasonable to me. Should be rare to encounter, since most calls to add should be with an argument that's statically known to be an object.

Only concern would be that the caller may not be set up to handle a thrown error.

Either way, let's leave it alone for now. Even if we did want to change it to a thrown error, we should do it in a separate change, and also apply the change to the other mutator methods too.

[adin-ttc]

I don't understand the history of cb but usually I would want to name that cb to match the event that the callback would fire.
For example all of these look like onCompleted callbacks where the caller will look to the props to see if there was an error to handle it. .. if refactoring all calls it hard then I would at least name the type of the callbacks the same so type CompletedCallback

They would all be the same and in this one case there is an optional value (in the case of a successful add there is a value) > you would only be coding the check for the value in the case of the add call anyway but at least they would all be consistent.. or i would specially call the type AddCompletedCallback

OR
Explicity create callback events for the events themselves but that would require more refactoring onError instead of "cb" and then add onSuccess in this case.. but i fear that would be more rework and the dual purpose of the callback even being fired would be harder to refactor

[ericyhwang]

This explains the conventions behind "Node-style" (error-first) callbacks:
https://gist.github.com/sunnycmf/b2ad4f80a3b627f04ff2

One benefit of that style is that it makes it obvious to the consumer that they need to explicitly do error handling. Compare to promises, where consumers may often use .then without remembering to handle rejections, which leads to unhandled promise rejections if not in an async function stack.

Downside is that style doesn't really play well with TypeScript typings:
#305 (comment)

[ericyhwang]

With this signature, a consumer won't be able to use traditional Node-style callback result handling in a type-safe way:

model.add(doc, (error, id) => {
  if (error) {
    return handleError(error);
  }
  // TS says `id` could be undefined
  console.log(id.length);
});

The types for Node's core libraries use a callback function type like:

(error: Error | null | undefined, value: T) => void

While that isn't strictly correct either, since it lets you use value without checking error, it matches better with how Node code has traditionally been written. It also only works because Node core libraries themselves aren't implemented in TS.

Unfortunately, TS doesn't have a great way of handling Node-style callbacks in a way that's type-safe for both the function implementation and consumer.
https://stackoverflow.com/questions/42603810/typescript-error-first-callback-typing-noimplicitany-strictnullchecks

That's all to say:

• The current approach is fine if we don't really care about TS consumers using the callback's id in the traditional way with type-safety.
• If we do care about that, then it's better to use the same signature Node does for the public API, and do as any shenanigans internally to make that work.

Honestly, it's probably fine to ignore, since consumers using the callback form can always get the id from the synchronous return value. Other thoughts welcome.

[craigbeck]

updated with non-optional value