hadronous / pic-js

An Internet Computer Protocol canister testing library for TypeScript and JavaScript.
https://hadronous.github.io/pic-js/
Apache License 2.0
11 stars 6 forks source link

createActor parameters pattern #46

Open peterpeterparker opened 6 months ago

peterpeterparker commented 6 months ago

Suggestion

I just used pic.createActor for the first time and, I was suprised to notice that the pattern used to pass its parameters is different that other function such as upgradeCanister or setupCanister. Therefore, I would like to suggest, even if it's a breaking change, to modify the signature of the method as follow.

From:

public createActor<T = ActorInterface>(
    interfaceFactory: IDL.InterfaceFactory,
    canisterId: Principal,
  ): Actor<T> {

To something similar to:

public createActor<T = ActorInterface>(
    {interfaceFactory, canisterId}: {
       interfaceFactory: IDL.InterfaceFactory,
       canisterId: Principal,
    }
  ): Actor<T> {

i.e. pass an object as parameter

WDYT?

peterpeterparker commented 6 months ago

A propos, maybe would also be interesting to add an optional identity to the parameters - e.g. this is how I had to use it:

const newActor = pic.createActor<SatelliteActor>(idlFactorSatellite, canisterId);
newActor.setIdentity(controller);
nathanosdev commented 6 months ago

My thinking here was that less than 3 paramters didn't justify an object as a parameter, but I can see how this feels inconsistent with the vast majority of public methods that have 3 or more parameters and hence have an object as a parameter.

The identity parameter is a nice suggestion too. I'll take a look at both next week and report back.

Thanks for the suggestion!

peterpeterparker commented 6 months ago

I have to add that I enforce object for all parameters in my projects for maintability reason and to prevent unexpected runtime error (when no candid or library such as Zod is used), so that's probably also why I was suprised. Definitely just a suggestion.