fix: add `AbortController` to the terminal types list

[micalevisk]

I've made this change on @nestjs/config: https://github.com/nestjs/config/pull/907 I found that it closes #46 as well

[micalevisk]

@Papooch do you mind adding the label hacktoberfest here? :smile: (or just the hacktoberfest topic to the repo)

[Papooch]

Thanks! Do you think that fix eliminates the need for the Terminal type altogether?

do you mind adding the label hacktoberfest here?

I added the hacktoberfest to the repo labels, is all there's to it?

[micalevisk]

is all there's to it?

yep, thanks :)

---

I tested the following without the Terminal; didn't worked

Also, those comments up there are the types that we got when using this PR's code

[Papooch]

Hm, it should not expand on the string methods 🤔. Does it also happen without your changes?

Also I'm wondering if we just put a constuctor type (new (...args: any[]) => any) to the TerminalType, whether that would fix most of the deep types issues. I don't see a reason to generate paths inside a class instance.

[micalevisk]

ok so without the changes I've made. I add that to the TerminalType but it didn't solved the infinite issue

[micalevisk]

(with this PR's changes)

as you could see, those any are due to the union with undefined on bar

[Papooch]

Hmm. I think we need to take a decision here. Even if you define all properties of the ClsStore as required, you can still get undefined when you try to retrieve them, if you don't populate all of them before accessing.

So if the types were to be 100% correct, then every cls.get should be return T | undefined. But that requires a lot of unnecessary null checks at the use-site, even if you know you have set up the store correctly, so that's why I chose this way.

Having an explicit nullable property in the store is something I didn't consider, but it makes sense actually.

I'm prety sure there's some typescript trickery that will still allow generating the paths for the nested properties, but make the return type nullable.

At the same time (and that's for a separate issue), we should never generate paths inside classes.

[micalevisk]

good point.

Here's the project (patched) that I'm playing with: https://gitlab.com/micalevisk/nestjs-cls-issue-46

just run npm ci and checkout the src/app.service.ts file.

I'll try to make them return undefined later

[micalevisk]

since ClsService#get could always return undefined, can't we just add an union with undefined below?:

https://github.com/Papooch/nestjs-cls/blob/d09c8e84a3a701cd80662ca0f8e7808d75533047/src/lib/cls.service.ts#L60

[micalevisk]

Another approach to avoid typing non-null assertions would be the same used in @nestjs/config: adding a optional flag type-arg on ClsService generics.

I can't think on anything other than those two :/

[micalevisk]

now we have

here's the TS playground

[Papooch]

Sorry for taking too long to respond, I finally found some time to look into it. It seem there is really no way to implicitly stop path generation inside instances of a class, but it seems that at least your fix prevents the typescript error, while still generating paths inside the AbortController, which is undesirable IMHO.

I am actually fine with adding AbortController to the TerminalType union if there's no way to weed out class instances automatically. Come to think of it, I'm fine with adding all JavaScript/Node.js built-ins - these are no different than Date or Map constructors.

As far as this PR goes, I think we can just merge it (unless you want to do some polishing) and address the other things in separate issues.

since ClsService#get could always return undefined, can't we just add an union with undefined below

I don't really believe in 100% type correctness at the expense of usability. As I said, this would pollute the code with null checks which add little value if your app is built around having stuff in the context. I'm more inclined towards your other suggestion with the strict flag. Moreover, I would probably straight up throw an exception by default, unless the strict flag is set to false - which should also change the return type. Then even the types would match.

However, either of these approaches deal with the possibility of nullable properties in the ClsStore. Either everything is optional, or everything is required unless you turn off strict. But since the store is user defined anyway, they should know how to work with it.

The typing of ClsService definitely needs some overhaul, but it will most likely result in a breaking change.

[micalevisk]

which is undesirable IMHO

oh, I misunderstood you before. Now I got it that the .get() should behave like Map#get, my bat.

I'll revert that so we could merge this now as a hot fix. And I'll try to improve the solution around your thoughts later, is it ok for you?

[micalevisk]

now we have this:

I guess that's it.

[Papooch]

I mean you didn't really have to remove the "stop if we found any". If anything, it prevent the error, which is nice. What I meant was that it was undesirable to generate paths inside AbortController, hence the suggestion to add it to TerminalType.