Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createActor parameters pattern #46

Open
peterpeterparker opened this issue Apr 30, 2024 · 3 comments
Open

createActor parameters pattern #46

peterpeterparker opened this issue Apr 30, 2024 · 3 comments

Comments

@peterpeterparker
Copy link

peterpeterparker commented Apr 30, 2024

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 peterpeterparker changed the title createActor parameters patter createActor parameters pattern Apr 30, 2024
@peterpeterparker
Copy link
Author

peterpeterparker commented Apr 30, 2024

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
Copy link
Contributor

nathanosdev commented Apr 30, 2024

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
Copy link
Author

peterpeterparker commented Apr 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants