build: move to monrepo #322

TimoGlastra · 2021-06-13T20:46:25Z

Basically ready to be merged.

Task list:

Split into @aries-framework/core, @aries-framework/node & @aries-framework/react-native
Tests working with new structure
Linting, prettier with monorepo
Automatic release script with lerna instead of release-it
update NPM token (to aries-framework org instead of hyperledger)

codecov-commenter · 2021-07-07T18:15:58Z

Codecov Report

Merging #322 (5d1143e) into main (55e8697) will decrease coverage by 1.89%.
The diff coverage is 87.27%.

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
- Coverage   88.32%   86.42%   -1.90%     
==========================================
  Files         239      234       -5     
  Lines        4796     4803       +7     
  Branches      615      750     +135     
==========================================
- Hits         4236     4151      -85     
- Misses        560      652      +92

Impacted Files	Coverage Δ
packages/core/src/agent/AgentMessage.ts	`86.66% <ø> (ø)`
packages/core/src/agent/BaseMessage.ts	`100.00% <ø> (ø)`
packages/core/src/agent/Dispatcher.ts	`96.42% <ø> (ø)`
packages/core/src/agent/EnvelopeService.ts	`100.00% <ø> (ø)`
packages/core/src/agent/Events.ts	`100.00% <ø> (ø)`
packages/core/src/agent/MessageReceiver.ts	`89.06% <ø> (ø)`
packages/core/src/agent/MessageSender.ts	`87.83% <ø> (ø)`
packages/core/src/agent/TransportService.ts	`92.30% <ø> (ø)`
packages/core/src/agent/helpers.ts	`100.00% <ø> (ø)`
...ges/core/src/agent/models/InboundMessageContext.ts	`93.33% <ø> (ø)`
... and 267 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55e8697...5d1143e. Read the comment docs.

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

TimoGlastra · 2021-07-14T09:38:15Z

Ready for review! I've coordinated with Ry to get everything set up for automatic releases so if we merge it we should automatically release under the new branch.

This PR doesn't introduce a lot of changes in how things works, except for the extraction of platform specific dependencies. Those are now the second parameter to agent.

import { Agent } from '@aries-framework/core'
import { agentDependencies } from '@aries-framework/node'

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

JamesKEbert

This looks good to me--there's a lot to look through on this, I left a couple of comments, but I may be missing some context behind the thoughts.
Thanks Timo for all of the effort here.

docs/setup-nodejs.md

docs/setup-react-native.md

packages/core/src/constants.ts

JamesKEbert · 2021-07-14T21:07:31Z

packages/react-native/tsconfig.build.json

+
+  "compilerOptions": {
+    "outDir": "./build",
+    // FIXME https://github.com/hyperledger/aries-framework-javascript/pull/327


Available for fixing?

Yes and no. It's really a hassle to not depend on any global node/react-native/browser types. We want types for fetch and WebSocket. We can either do that by depending on types for a specific platform, or we should write them ourselves...

Hmmm.. can we rely on global Node types, and then specify/override those in the respective environments? (my knowledge is a little light here so)

I would rather avoid writing our own types, but I understand it's a tricky problem 🙁

tests/e2e.test.ts

jakubkoci

That's Awesome 🎉 Great job Timo.

I added just a few insignificant notes :)

.eslintrc.js

jakubkoci · 2021-07-15T10:07:35Z

packages/core/package.json

+    "@types/bn.js": "^5.1.0",
+    "@types/events": "^3.0.0",
+    "@types/indy-sdk": "^1.16.5",
+    "@types/node-fetch": "^2.5.10",


I guess we will eventually remove this from the core, right?

Yes once we resolve the typing issues as described above (declare them ourselves or use some trick to make it work)

jakubkoci · 2021-07-15T10:10:01Z

packages/core/src/agent/Agent.ts

-    // File system differs based on NodeJS / React Native
-    this.container.registerInstance(InjectionSymbols.FileSystem, this.agentConfig.fileSystem)
+    // Platform specific dependencies
+    this.container.registerInstance(InjectionSymbols.FileSystem, new dependencies.FileSystem())


Nitpick: What about creating an instance outside? Then you will have more flexibility to pass params into the constructor or set the instance as you need before passing it into the agent

I'll look at that in a separate PR. That would mean extra steps to get started, even though you won't need in 99% of the cases

We can make it an optional config property?

jakubkoci · 2021-07-15T10:11:41Z

packages/core/src/agent/AgentDependencies.ts

+import type fetch from 'node-fetch'
+import type WebSocket from 'ws'


Maybe you already mention something about thsese two deps. Would it be possible to remove it later? In other words. Just mentioning it in case we forgot it here :)

Yes. But we only depend on the @types packages. Not the actual packages themselves

jakubkoci · 2021-07-15T10:19:15Z

packages/core/src/transport/WsOutboundTransporter.ts

    this.logger.debug('Starting WS outbound transport')
+    this.WebSocket = agentConfig.agentDependencies.WebSocket


The this.WebSocket could be hard to understand what it means 🤔 like what's actually behind that property. Just saying, don't have any idea for improvement right now. 😄

I've renamed to WebSocketClass for now. Let me know if you want something else

jakubkoci · 2021-07-15T10:26:04Z

packages/react-native/tsconfig.build.json

+
+  "compilerOptions": {
+    "outDir": "./build",
+    // FIXME https://github.com/hyperledger/aries-framework-javascript/pull/327


I would rather avoid writing our own types, but I understand it's a tricky problem 🙁

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

TimoGlastra changed the base branch from monorepo to main June 13, 2021 20:46

TimoGlastra mentioned this pull request Jun 17, 2021

fix: remove dependency on global types #327

Merged

This was linked to issues Jun 22, 2021

Remove platform specific dependencies from the framework #306

Closed

Moving to a monorepo #270

Closed

TimoGlastra force-pushed the monorepo branch 2 times, most recently from c0d4abb to 7d17038 Compare July 7, 2021 18:06

TimoGlastra mentioned this pull request Jul 7, 2021

refactor: inject transport service in transports #368

Merged

TimoGlastra force-pushed the monorepo branch from 7d17038 to 7cc53df Compare July 7, 2021 19:02

refactor: change repo to monorepo

8efe87e

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

TimoGlastra force-pushed the monorepo branch from 7cc53df to 8efe87e Compare July 7, 2021 20:30

TimoGlastra added 10 commits July 13, 2021 20:34

Merge remote-tracking branch 'upstream/main' into monorepo

f7ac970

fix: conflicts after merge

531729d

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

fix: more issues after merge

25e55ce

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

build: fix issue with borc types

7b9c793

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

fix: absolute imports

ef7d0f1

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

refactor: remove unused import

009c7cf

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

Merge remote-tracking branch 'upstream/main' into monorepo

0a4f2a3

test: add genesis path to default config

fc73ee4

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

fix: logging before assigned

fe56d1f

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

test: fix e2e tests

e7315f0

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

TimoGlastra marked this pull request as ready for review July 14, 2021 09:28

TimoGlastra requested a review from a team as a code owner July 14, 2021 09:28

TimoGlastra assigned jakubkoci and JamesKEbert and unassigned jakubkoci and JamesKEbert Jul 14, 2021

TimoGlastra requested review from jakubkoci and JamesKEbert July 14, 2021 09:29

JamesKEbert approved these changes Jul 14, 2021

View reviewed changes

jakubkoci approved these changes Jul 15, 2021

View reviewed changes

TimoGlastra added 4 commits July 15, 2021 21:12

fix: updates based on pr feedback

4e686f5

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

Merge remote-tracking branch 'upstream/main' into monorepo

6bdf7a3

fix: issues with endpoints and transports

534cfc4

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

test: remove react-native import for now

5d1143e

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

TimoGlastra force-pushed the monorepo branch from cd98cdd to 5d1143e Compare July 15, 2021 20:36

TimoGlastra merged commit ffaa3c0 into openwallet-foundation:main Jul 15, 2021

TimoGlastra deleted the monorepo branch July 15, 2021 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: move to monrepo #322

build: move to monrepo #322

TimoGlastra commented Jun 13, 2021 •

edited

Loading

codecov-commenter commented Jul 7, 2021 •

edited

Loading

TimoGlastra commented Jul 14, 2021

JamesKEbert left a comment

JamesKEbert Jul 14, 2021

TimoGlastra Jul 14, 2021 •

edited

Loading

JamesKEbert Jul 14, 2021

jakubkoci Jul 15, 2021

jakubkoci left a comment

jakubkoci Jul 15, 2021

TimoGlastra Jul 15, 2021

jakubkoci Jul 15, 2021

TimoGlastra Jul 15, 2021

jakubkoci Jul 15, 2021

TimoGlastra Jul 15, 2021

jakubkoci Jul 15, 2021

TimoGlastra Jul 15, 2021

jakubkoci Jul 15, 2021

		import type fetch from 'node-fetch'
		import type WebSocket from 'ws'

		this.logger.debug('Starting WS outbound transport')
		this.WebSocket = agentConfig.agentDependencies.WebSocket

build: move to monrepo #322

build: move to monrepo #322

Conversation

TimoGlastra commented Jun 13, 2021 • edited Loading

codecov-commenter commented Jul 7, 2021 • edited Loading

Codecov Report

TimoGlastra commented Jul 14, 2021

JamesKEbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubkoci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented Jun 13, 2021 •

edited

Loading

codecov-commenter commented Jul 7, 2021 •

edited

Loading

TimoGlastra Jul 14, 2021 •

edited

Loading