Improve return types for render, renderAsync, renderFile and renderFileAsync

[nhaef]

Hey, I really like eta and I have been using it for a few projects already. I would love to contribute to this project. ❤

As already explained in this issue, the types of the render functions can be improved by overloading them. There are essentially three branches and every branch defines their respective return type:

export default function render(
    template: string | TemplateFunction,
    data: object,
    config?: PartialConfig,
    cb?: CallbackFn
): string | Promise<string> | void {
    if (options.async) {
        if (cb) {
            // user provides callback function
            // return type is void
        } else {
            // user sets async option
            // return type is Promise<string>
        }
    } else {
        // return type is string
    }
}

The idea would be to define one function overload per return type so that the function can be consumed easily, without the need of any type assertion. The implementation signature is provided as well in order to prevent breaking changes.

I appreciate any feedback, ideas and suggestions.

This PR resolves #189.

[nebrelbug]

@nhaef thanks for working on this! I think this change will be really helpful. Is it ready for review yet?

[nhaef]

Hi @nebrelbug, thanks for the feedback. It isn't ready for review yet, but it is almost there :-) I will update this PR later today

[nhaef]

Okay, function overloads for renderFile and renderFileAsync have been added as well. The return type of both functions depends on the return type of tryHandleCache. This method is pretty much structured like this:

function tryHandleCache(data: object, options: EtaConfigWithFilename, cb: CallbackFn | undefined) {
    if (cb) {
        // return type is void
    } else {
        // return type is Promise<string>
    }
}

The function overloads of renderFile and renderFileAsync are specified so that the return type is void if the user provides a CallbackFn, otherwise the return type is Promise<string>. Similar to the previous changes, I did not remove the existing function overloads with the optional CallbackFn so that this PR does not introduce any breaking changes.

[nhaef]

Here is a preview of the inferred types.

click me

Eta.render

Eta.render(templateFn, data); // string
Eta.render(templateFn, data, config); // string
Eta.render(templateFn, data, { ...config, async: true }); // Promise<string>
Eta.render(templateFn, data, { ...config, async: true }, callbackFn); // void

Eta.renderAsync

Eta.renderAsync(templateFn, data); // Promise<string>
Eta.renderAsync(templateFn, data, config); // Promise<string>
Eta.renderAsync(templateFn, data, { ...config, async: true }); // Promise<string>
Eta.renderAsync(templateFn, data, { ...config, async: true }, callbackFn); // void

Eta.renderFile

Eta.renderFile(fileName, data, config); // Promise<string>
Eta.renderFile(fileName, data, config, callbackFn); // void
Eta.renderFile(fileName, { ...data, ...config }); // Promise<string>
Eta.renderFile(fileName, { ...data, ...config }, callbackFn); // void

Eta.renderFileAsync

Eta.renderFileAsync(fileName, data, config); // Promise<string>
Eta.renderFileAsync(fileName, data, config, callbackFn); // void
Eta.renderFileAsync(fileName, { ...data, ...config }); // Promise<string>
Eta.renderFileAsync(fileName, { ...data, ...config }, callbackFn); // void

[nhaef]

The changes are now ready for review. I would especially be interested if I should add JSDoc annotations for each function overload or not. Looking forward to your feedback 🙂

[nebrelbug]

@nhaef thanks so much for your work here! Sorry I've just gotten to reviewing this now. As I commented above, it seems to me that we should be able to remove some redundant function overloads. Is that correct?

[nhaef]

@nhaef thanks so much for your work here! Sorry I've just gotten to reviewing this now. As I commented above, it seems to me that we should be able to remove some redundant function overloads. Is that correct?

No problem and thanks for the review :) And also sorry from my side as well, I was busy the last few days.

I removed some unnecessary function overloads and updated the conversations. I think some of the conversations which are still open can be summed up with the question "Should we make the implementation signature visible for backwards compatibility?".

[nebrelbug]

@nhaef I actually didn't realize that implementation signatures were not visible to users. With that knowledge, this PR looks great. I'll merge right now!

[nebrelbug]

@nhaef do you mind if I add you to the contributors section in the README?

[nhaef]

@nhaef do you mind if I add you to the contributors section in the README?

not at all, I'd be honored :D

[nebrelbug]

@all-contributors please add @nhaef for code

[allcontributors]

@nebrelbug

I've put up a pull request to add @nhaef! 🎉