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

feat!: agent module registration api #955

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Jul 20, 2022

Signed-off-by: Timo Glastra [email protected]

Add agent module registration api. With this you can now register custom modules on the agent like this:

const agent = new Agent({
  config: { /* agent config */ },
  modules: {
    tenants: new TenantsModule(),
  },
  dependencies: agentDependencies,
})

This will register the tenants module and make the tenants api available. There's some nice typescript magic going on here that automatically infers the api type from the tenants module, allowing the agent to be fully typed with custom modules:

// agent.modules.tenants is fully typed
const tenantAgent = await agent.modules.tenants.getTenantAgent({ tenantId: 'the-tenant-id' })

// tenantAgent is also fully typed, but doesn't have the `agent.api.tenants` available
await tenantAgent.oob.createInvitation()

As you may have already noticed from the example above, all the custom module api's are now nested under the agent.modules property. I've taken this approach for two reasons:

You can define the api for a module using the api key on a module:

class MyModule {
  public api = MyApi

  public register(dependencyManager: DependencyManager) {
    dependencyManager.registerContextScoped(MyApi)
  }
}

All modules that were registered by default on the agent are still registered by default. So agent.oob is present, as well as agent.connections, etc... For now the default modules are still initialized based on the configuration in the agent config, but in a next PR I'll remove that logic and you will only be able to configure modules using the module config itself. If you want to provide custom configuration for a default module you can do so by registering the module. This will just register the module with you custom config, instead of using the default config:

const agent = new Agent({
  config: { /* agent config */ },
  modules: {
    // provide custom config for the default registered connections module.
    connections: new ConnectionsModule({
        autoAcceptConnections: true
    })
  },
  dependencies: agentDependencies,
})

BREAKING CHANGE: custom modules have been moved to the .modules namespace. In addition the agent constructor has been updated to a single options object that contains the config and dependencies properties. Instead of constructing the agent like this:

const agent = new Agent({ /* config */ }, agentDependencies)

You should now construct it like this:

const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies
})

This allows for the new custom modules to be defined in the agent constructor.

@TimoGlastra TimoGlastra added modularization Tasks related to modularization of the framework breaking change PR that introduces a breaking change 0.3.0 Changes for the 0.3.0 release labels Jul 20, 2022
@TimoGlastra TimoGlastra requested a review from a team as a code owner July 20, 2022 19:53
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #955 (d6d524f) into 0.3.0-pre (273e353) will increase coverage by 0.03%.
The diff coverage is 99.21%.

@@              Coverage Diff              @@
##           0.3.0-pre     #955      +/-   ##
=============================================
+ Coverage      88.20%   88.23%   +0.03%     
=============================================
  Files            636      637       +1     
  Lines          15036    15092      +56     
  Branches        2536     2542       +6     
=============================================
+ Hits           13262    13316      +54     
- Misses          1676     1678       +2     
  Partials          98       98              
Impacted Files Coverage Δ
packages/core/src/index.ts 100.00% <ø> (ø)
packages/core/src/modules/oob/OutOfBandApi.ts 87.73% <ø> (ø)
packages/core/src/plugins/Module.ts 50.00% <ø> (ø)
packages/core/src/plugins/DependencyManager.ts 90.32% <90.90%> (-1.35%) ⬇️
packages/core/src/agent/Agent.ts 97.34% <100.00%> (-0.26%) ⬇️
packages/core/src/agent/AgentModules.ts 100.00% <100.00%> (ø)
packages/core/src/agent/BaseAgent.ts 95.40% <100.00%> (+0.05%) ⬆️
.../src/modules/basic-messages/BasicMessagesModule.ts 100.00% <100.00%> (ø)
.../core/src/modules/connections/ConnectionsModule.ts 100.00% <100.00%> (ø)
.../core/src/modules/credentials/CredentialsModule.ts 100.00% <100.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@karimStekelenburg
Copy link
Contributor

Nice job @TimoGlastra! My only remark here is that the agent.api naming doesn't make total sense to me (maybe I'm seeing it wrong though). Wouldn't agent.modules make more sense?

@TimoGlastra
Copy link
Contributor Author

My only remark here is that the agent.api naming doesn't make total sense to me (maybe I'm seeing it wrong though). Wouldn't agent.modules make more sense?

Yeah have been thinking about using module, but that also didn't fully make sense to me. It only exposes the api of the module, not the whole module object (so ConnectionsApi class instance, not the ConnectionsModule instance)

@karimStekelenburg
Copy link
Contributor

karimStekelenburg commented Jul 20, 2022

Haha fair, this is a very semantic thingy.

My thought here is this: as a user I will pass modules to the agent constructor. Therefore, I already know that a module adds certain functionality. However, I might not be familiar with the internal structure of modules, so I might not be aware of the concept of FooApi classes.

Since I'm passing modules, I wouldn't be surprised to have to access their APIs through agent.modules.

Anyway, just my thought! I try to think about these things as an outsider, not a framework developer.
This maybe is a good candidate for a WG discussion ;)

@TimoGlastra
Copy link
Contributor Author

Input from WG call:

  • add custom property that only contains the custom modules. Default modules will still be top level.
  • use modules instead of api.

I think I agree with the agent.modules part over the agent.api. Not sure I'm a fan of the custom namespace. More feedback is welcome!

@berendsliedrecht
Copy link
Contributor

Input from WG call:

  • add custom property that only contains the custom modules. Default modules will still be top level.

  • use modules instead of api.

I think I agree with the agent.modules part over the agent.api. Not sure I'm a fan of the custom namespace. More feedback is welcome!

I am slightly in favour of the top-level modules and custom modules defined in agent.modules. I think doing something like agent.modules.credentials.offerCredential becomes so long that it will be annoying to use. Custom modules are IMHO second-class and can be defined under a specific namespace.

Keeping the core-defined modules top level, which in the end might not be a lot, makes a cleaner API.

Having said that, doing agent.oob.XXX For an invitation and then agent.modules.credential.XXX For issuance is very weird.

I am fine with either for different reasons. Maybe we can look at different frameworks and how they solved this issue?

@TimoGlastra TimoGlastra added this to the v0.3.0 milestone Jul 25, 2022
@berendsliedrecht
Copy link
Contributor

I thought about this issue a bit more, and it might not be such a big issue to have breaking changes when we add new modules into core. Adding a module into core, after modularisation, is very unlikely to happen when we have the did core modules in there. Unless they expand their spec or we want add seperation, we would not have the add any modules. Of course, it might happen and bundling it with some other breaking changes would be fine IMHO.

Also, as we mentioned in the last WG call, the agent.module.XXX does not really solve the key-collision issue vs the agent.XXX. With this my preference is for agent.XXX unless I am missing something that was another purpose of agent.module.XXX.

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Aug 12, 2022

Well we need to change the call to Agent.create (or something like that such as a separate method createAgent) instead of calling new Agent(), but otherwise there's no difference.

I'll go with the top level approach then!

@genaris
Copy link
Contributor

genaris commented Aug 12, 2022

I don't see any problem on using an agent creator method, especially considering that we currently need to call initialize method after the constructor to make it usable. Could be done anything there to simplify this initialization in cases where applicable?

A question that arises from this approach is if there would be a chance of overriding a core module. We currently do so with connections module to add some project-specific capabilities (we sub-class Agent and define a connections property assigned to a sub-class of ConnectionModule), and even if it feels a bit hacky, it works quite smoothly.

@berendsliedrecht
Copy link
Contributor

berendsliedrecht commented Aug 12, 2022

I don't see any problem on using an agent creator method, especially considering that we currently need to call initialize method after the constructor to make it usable. Could be done anything there to simplify this initialization in cases where applicable?

I think await Agent.create({}).initialize() could also be await Agent.createAndInitialize({}). Seems quite convenient as I think that creating the Agent and initialising it afterwards is something new users might not know.

Also, again not a massive fan of bloating the config, but adding a value to the config with initializeOnCreation: Boolean might also avoid it. However, this might cause issues as Agent.create({}) is synchronous and Agent.initialize is asynchronous and I am not sure if we can handle this properly, return type wise (void | Promise), based on the config field.

A question that arises from this approach is if there would be a chance of overriding a core module. We currently do so with connections module to add some project-specific capabilities (we sub-class Agent and define a connections property assigned to a sub-class of ConnectionModule), and even if it feels a bit hacky, it works quite smoothly.

It would be possible to override a core module.

const agent = Agent.create({
  config: { /* agent config */ },
  modules: {
    CORE_MODULE: new CORE_MODULE({config: 'yes'}), // CORE_MODULE is a core module and here we can define a new one.
  },
  dependencies: agentDependencies,
})

@TimoGlastra
Copy link
Contributor Author

Yes as berend mentioned it would be possible to override the core module by providing it in the agent config, however the typing is currently set to only allow the same module. You would have to write your CustomModule and register it under a different key. We could loosen the type a bit to allow different modules, but I think it makes sense to use a different key if you're not using the connections module. So agent.myConnections or something that contains your business name etc.. would be a bit more explicit maybe? Otherwise we can look at making it accept the same interface (as it extends the connections module)

@genaris
Copy link
Contributor

genaris commented Aug 16, 2022

Yes as berend mentioned it would be possible to override the core module by providing it in the agent config, however the typing is currently set to only allow the same module. You would have to write your CustomModule and register it under a different key. We could loosen the type a bit to allow different modules, but I think it makes sense to use a different key if you're not using the connections module. So agent.myConnections or something that contains your business name etc.. would be a bit more explicit maybe? Otherwise we can look at making it accept the same interface (as it extends the connections module)

Certainly, if it can accept the same interface (or an extension of it) it would be great to allow these 'tuned core modules'. Of course there is no problem on using other keys, but if I expose agent.connections and agent.myConnections I'll allow my consumers to use both, and probably that's not something I want.

Anyway, there are some tricks in TypeScript that allow hiding some keys from superclasses, and for sure it's possible to think things differently for using different keys in an elegant way, so it's not a big deal if this idea gets too complicated. Just a 'nice to have' from my point of view.

@TimoGlastra TimoGlastra force-pushed the 0.3.0-pre branch 3 times, most recently from f2e61d8 to f38ac05 Compare August 26, 2022 19:09
BREAKING CHANGE: all modules have been moved to the .api namespace. Instead of using agent.xxx (e.g. agent.oob) you should now call agent.api.oob instead. In addition the agent constructor has been updated to a single options object that contains the `config` and `dependencies` properties. Instead of constructing the agent like this:
```ts
const agent = new Agent({ /* config */ }, agentDependencies)
```

You should now construct it like this:
```ts
const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies
})
```

This is to alow for the new custom modules that can be befined in the agent constructor.
`

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra force-pushed the feat/module-registration branch from 4dde7b0 to 2b4a740 Compare September 8, 2022 18:09
@TimoGlastra
Copy link
Contributor Author

it's not a big deal if this idea gets too complicated. Just a 'nice to have' from my point of view.

I think it's doable, but maybe for a future PR. This can be added as a non-breaking change, so we could maybe push it to 0.3.1

@TimoGlastra
Copy link
Contributor Author

This PR is now updated and should be ready to merge. I've updated the description of the PR but it boils down to:

  • default modules are still nested under agent.xxx
  • custom modules are nested under agent.modules.xxx.

I think this is one of the blockers for the 0.3.0 release, so let's get this merged :)

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra requested a review from genaris September 8, 2022 18:15
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Really love this new module registration API. There are still some doubts to solve in the path of AFJ modularization, but certainly this is an excellent step forward to reach it.

Note: there are a few warnings from GitHub Actions validation

@TimoGlastra TimoGlastra merged commit 82a17a3 into openwallet-foundation:0.3.0-pre Sep 10, 2022
@TimoGlastra TimoGlastra deleted the feat/module-registration branch September 10, 2022 17:59
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
Signed-off-by: Timo Glastra <[email protected]>

BREAKING CHANGE: custom modules have been moved to the .modules namespace. In addition the agent constructor has been updated to a single options object that contains the `config` and `dependencies` properties. Instead of constructing the agent like this:

```ts
const agent = new Agent({ /* config */ }, agentDependencies)
```

You should now construct it like this:
```ts
const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies
})
```

This allows for the new custom modules to be defined in the agent constructor.

Signed-off-by: Ariel Gentile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.3.0 Changes for the 0.3.0 release breaking change PR that introduces a breaking change modularization Tasks related to modularization of the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants