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(action-menu)!: move to separate package #1049

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Oct 11, 2022

Signed-off-by: Ariel Gentile [email protected]

BREAKING CHANGE: action-menu module has been removed from the core and moved to a separate package. To integrate it in an Agent instance, it can be injected in constructor like this:

const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies,
  modules: {
    actionMenu: new ActionMenuModule(),
    /* other custom modules */
   }
})

Then, module API can be accessed in agent.modules.actionMenu.

@genaris genaris requested a review from a team as a code owner October 11, 2022 21:57
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Merging #1049 (4abcc4b) into 0.3.0-pre (97d3073) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##           0.3.0-pre    #1049      +/-   ##
=============================================
- Coverage      88.22%   88.18%   -0.05%     
=============================================
  Files            680      680              
  Lines          15852    15828      -24     
  Branches        2548     2548              
=============================================
- Hits           13986    13958      -28     
- Misses          1861     1865       +4     
  Partials           5        5              
Impacted Files Coverage Δ
packages/action-menu/src/ActionMenuEvents.ts 100.00% <ø> (ø)
packages/action-menu/src/ActionMenuRole.ts 100.00% <ø> (ø)
packages/action-menu/src/ActionMenuState.ts 100.00% <ø> (ø)
...n-menu/src/errors/ActionMenuProblemReportReason.ts 100.00% <ø> (ø)
...enu/src/handlers/ActionMenuProblemReportHandler.ts 100.00% <ø> (ø)
...ges/action-menu/src/handlers/MenuMessageHandler.ts 100.00% <ø> (ø)
...ion-menu/src/handlers/MenuRequestMessageHandler.ts 100.00% <ø> (ø)
.../action-menu/src/handlers/PerformMessageHandler.ts 100.00% <ø> (ø)
packages/action-menu/src/handlers/index.ts 100.00% <ø> (ø)
packages/action-menu/src/index.ts 100.00% <ø> (ø)
... and 23 more

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

</p>
<br />

TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of todo, could we add a very simple installation + how to add it as a module (basically install + what you have in the breaking change message).

We should also do this for question answer

"class-validator": "0.13.1"
},
"devDependencies": {
"@aries-framework/core": "0.2.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have it as a dev dependencie if it is already a dependency, right?

Also, I think it should be a peer dep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. And regarding the version number, should it always match the version of AFJ core/node/react-native?

For the moment it's 0.2.4 but I guess we'll need to update to 0.3.0 for all packages in a further PR before merging to main.

}

const publicDidSeed = process.env.TEST_AGENT_PUBLIC_DID_SEED ?? '000000000000000000000000Trustee9'
export function getBaseConfig(name: string, extraConfig: Partial<InitConfig> = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can import these from the helpers.ts test file directly. We should look at creating an internal/private @aries-framework/test-utils package over time, but for now the direct import works fine I think. Otherwise we're going to duplicate the same setup a lot of times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Actually it seems that it was a copy-paste issue from question-answer, as I'm not using it. :-) Will update it in question-answer as well.

Signed-off-by: Ariel Gentile <[email protected]>
@genaris genaris requested a review from TimoGlastra October 12, 2022 12:39
Signed-off-by: Ariel Gentile <[email protected]>
@@ -6,7 +6,7 @@
height="250px"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not as a comment on your work, but we should probably export the dummyModule so other people can get started aswell :).

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean exactly by this @blu3beri?

Copy link
Contributor

@berendsliedrecht berendsliedrecht Oct 12, 2022

Choose a reason for hiding this comment

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

We have a samples/extension-module in AFJ explaining to people on how to create an extension module, the old way. If we update that everyone has a clear example to start modularising other modules and their custom modules that they might use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha! Yes totally agree

@TimoGlastra TimoGlastra merged commit e0df0d8 into openwallet-foundation:0.3.0-pre Oct 12, 2022
@genaris genaris deleted the feat/extract-action-menu-module branch October 12, 2022 16:08
genaris added a commit to 2060-io/aries-framework-javascript that referenced this pull request Oct 13, 2022
)

Signed-off-by: Ariel Gentile <[email protected]>

BREAKING CHANGE: action-menu module has been removed from the core and moved to a separate package. To integrate it in an Agent instance, it can be injected in constructor like this:
```ts
const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies,
  modules: {
    actionMenu: new ActionMenuModule(),
    /* other custom modules */
   }
})
```

Then, module API can be accessed in `agent.modules.actionMenu`.

chore: fix merge errors

Signed-off-by: Ariel Gentile <[email protected]>

chore: revert snapshot

Signed-off-by: Ariel Gentile <[email protected]>

chore: add new line

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants