dbos-inc / dbos-transact

The TypeScript framework for backends that scale
https://docs.dbos.dev
MIT License
291 stars 20 forks source link

change any to unknown where possible #472

Closed demetris-manikas closed 1 month ago

demetris-manikas commented 1 month ago

A lot wiser now. :)

chuck-dbos commented 1 month ago

Long story but as a result of some of the merge conflicts hit when the prior PR got merged and unmerged, I left some of these changes on my branch.

My observations: Changing the proxy to Record is fine Workflow, Communicator, and Transaction functions should be typed with <unknown[], unknown> (when unknown), because it's always an array of inputs and one output. This gets rid of a lot of it.

demetris-manikas commented 1 month ago

Can you please elaborate a bit on what you mean by "Workflow, Communicator, and Transaction functions should be typed with <unknown[], unknown> (when unknown)" and what changes should I add or remove?

Up until yesterday I had the impression that changing any to unknown only makes the code safer. Turned out that this is is not the case for the '...rest' arguments of functions in type/interface declarations. type <T extends any[]>Func(a: string, ...args:T) => void is not equivalent to type <T extends unknown[]>Func(a: string, ...args:T) => void

chuck-dbos commented 1 month ago

Up until yesterday I had the impression that changing any to unknown only makes the code safer.

Not sure about "safer". It changes what the compiler makes you acknowledge when you use the item, but doesn't change what the code does at runtime. The main thing is that the args are an array. There were some places where it was treated as anything, array or not, wonder if that was to conserve keystrokes.

demetris-manikas commented 1 month ago

Typescript is not type safe, It is just a good linter to minimize stupid errors. Having written a lot of both I can assure you that typescript code produces a lot less of those. The way to make it closer to a typed language is to use libraries like 'zod' or 'class-validator' everywhere like nestjs does.

Typescript in the backend is an error in my opinion but it has become the norm due to fashion and availability of programmers but what can you do.

Still any[] instead of plain any will save you from passing a non array into a function that expects an array.

Otherwise just write plain javascript and enjoy the nightmares that go along with it. :)

Still I don't understand what you meant by your comment. Please specify. If I have made a typo writing anyinstead of any[] just tell me where and I will fix it. To avoid more of this it would be probably be better to comment straight on the code so that we do not have to go back and forth. Cheers

demetris-manikas commented 1 month ago

Also having a file starting with /* eslint-disable @typescript-eslint/no-explicit-any */ is just bad practice.

Avoiding bad practices will let you sleep more during nights :)

Also look at the change at runtime.ts. Changing any to unknown revealed a 'stupid' mistake of casting a value to a promise. Just because there is no Promise related call on modules this code runs fine but if one tried to do something like modules.then() then it would blow.

I am trying to remove all these avoidances of typescript and linting cause I have seen a lot of these firing back. I fully understand that

     ? // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
      (...args: any[]) => this.transaction(op.registeredFunction as Transaction<any[], any>, ...args)

in not different to

      ? (...args: unknown[]) => this.transaction(op.registeredFunction as Transaction<unknown[], unknown>, ...args)

But having as little // eslint-disable-next-line is a good practice and it should be followed whenever possible.

chuck-dbos commented 1 month ago

Right, what I am saying is I kept some of the changes in #460 and don't want another pile of merge conflicts with it.

demetris-manikas commented 1 month ago

Fair. Long standing PR's IS an nightmare.. I 've been there and it is not a nice place... Feel free to reject this one. I will revisit the //es-lint-disables at a later time if I have nothing else to do

demetris-manikas commented 1 month ago

@chuck-dbos You can incorporate any of these changes to your branch if you wish. Like you said there is very little valuable stuff in this PR. Closing...