fix: di and config mgmt #38

jleach · 2025-01-17T19:21:52Z

This PR makes the following changes:

Fixes Unable to run mediator #34: I diffed the work done by Credebl and this repo. Looks like the problem was caused some imports using the word type. I've removed all the ones causing problems and left the others.
Moved constants to config: I moved some constants to a config file so they can be easily exposed in Helm charts. This allows Kubernetes to use environment variables for configuration.
Added dotenv support: I added support for dotenv so that developers can use environment variables in development environments. In production, the library will load from process.env.*.
biome rules: The biome rule useImportType was causing issues so I disabled it.

Signed-off-by: Jason C. Leach <[email protected]>

jleach · 2025-01-17T19:54:40Z

@TimoGlastra This runs fine. I still need to fix the style errors. Wanted to put it up for early review since I kinda went rogue in fixing #34. Thoughts?

TimoGlastra

Generally ok, but could use some cleanup.

Haven't used nconf before but i like the config file based approach.

TimoGlastra · 2025-01-17T19:47:10Z

package.json

+    "dotenv": "^16.4.7",
    "express": "^4.21.2",
+    "nconf": "^0.12.1",
+    "node-fetch": "^2.6.11",


I generally try to keep dependencies to a minimum. Makes maintenance easier. Node 18 has fetch built in. Node 20+ i think (we can keep that as a minimum i think?) has env file support built in.

I just removed node fetch before migration.

TimoGlastra · 2025-01-17T19:48:16Z

src/agent.ts

@@ -48,14 +50,14 @@ export async function createAgent() {
  const app = express()
  const socketServer = new Server({ noServer: true })

-  const logger = new Logger(LOG_LEVEL)
+  const logger = new Logger(config.get('agent:logLevel'))


Does this have typing support for config.get?

Otherwise i'd rather export these from a config file so we have correct typing

No typing unfortunately. A bit more on this in my latest comment. I can go either way.

TimoGlastra · 2025-01-17T19:48:56Z

src/agent.ts

@@ -104,6 +106,7 @@ export async function createAgent() {
    res.status(200).send('Ok')
  })

+  // eslint-disable-next-line @typescript-eslint/no-misused-promises


We don't use eslint

TimoGlastra · 2025-01-17T19:49:25Z

src/config.ts

+import dotenv from 'dotenv';
+import nconf from 'nconf';
+import path from 'path';
+// import { fileURLToPath } from 'url';


Can you remove the commented code?

TimoGlastra · 2025-01-17T19:50:03Z

src/index.ts


+console.log(`AGENT_PORT: ${config.get('agent:port')}`)


Can you remove the console logs? It should only log if the logger is enabled

TimoGlastra · 2025-01-17T19:53:18Z

src/storage/StorageMessageQueue.ts

-import type { MessageRepository } from './MessageRepository'
+import { MessageRepository } from './MessageRepository'
+import { PushNotificationsFcmRepository } from '../push-notifications/fcm/repository'
+import fetch from 'node-fetch'


No need for node fetch

TimoGlastra · 2025-01-17T19:53:31Z

src/storage/StorageMessageQueue.ts

@@ -137,8 +144,9 @@ export class StorageServiceMessageQueue implements MessagePickupRepository {
  private async processNotification(message: NotificationMessage) {
    try {
      const body = {
-        fcmToken: message.token,
+        fcmToken: message.token || 'abc',


TimoGlastra · 2025-01-17T19:53:39Z

src/storage/StorageMessageQueue.ts

        messageType: message.messageType,
+        clientCode: message.clientCode || '5b4d6bc6-362e-4f53-bdad-ee2742bc0de3',


TimoGlastra · 2025-01-17T19:54:09Z

src/storage/StorageMessageQueue.ts

+        token: pushNotificationFcmRecord?.deviceToken || '',
+        clientCode: pushNotificationFcmRecord?.clientCode || '',


Suggested change

token: pushNotificationFcmRecord?.deviceToken || '',

clientCode: pushNotificationFcmRecord?.clientCode || '',

token: pushNotificationFcmRecord?.deviceToken,

clientCode: pushNotificationFcmRecord?.clientCode,

TimoGlastra · 2025-01-17T20:03:05Z

src/config.ts

+// nconf.defaults({});
+
+if (env === 'development') {
+  console.log('Configuration:', nconf.get());


Console.log

sairanjit · 2025-01-17T20:15:58Z

@jleach You can remove the clientCode as it was built for some custom logic and can be removed now.

Signed-off-by: Jason C. Leach <[email protected]>

jleach · 2025-01-17T22:03:51Z

This pr is undoing quite a few things i added. Can you check to only integrate needed changes from credebl repo?

@TimoGlastra For sure. That was just a draft for early feedback.

I wen't through and diffed the two repos. Looks like #34 was caused by some import type { PushNotificationsFcmService } from './services/PushNotificationsFcmService' where key word type was causing the problem. I went through and removed all the ones that were causing the errors, then when I ran biome to fix any formatting issues it added them back. I've disabled the problematic rule for now.

This PR is mostly the config changes. I'm Ok with a more simple approach:

import dotenv from "dotenv";

if (env === 'development') {
  dotenv.config();
}

type Config = {
  PORT: number;
  NODE_ENV: string;
}

const config: Config = {
  PORT: parseInt(process.env.PORT || "3000", 10),
  NODE_ENV: process.env.NODE_ENV || "development",
};

The up-side to nconf is that you can use a config file like config.json which can be handy, especially in Kubernets where you often use a ConfigMap for non-confidential data.

{
  "db": {
    "host": "foo.example.ca",
    ...
  },
  "agent": {
    "port": 8888,
    ...
  },
  "wallet": {
    "name": "my-cool-wallet",
    ...
  }
}

Also, I couldn't find the option --env-file in ts-node so I changed this and added dotenv:

./node_modules/.bin/ts-node --env-file .env src/index.ts
...
Error: Unknown or unexpected option: --env-file

node ➜ /work (fix/di-and-config) $ ./node_modules/.bin/ts-node -v
v10.9.2

Signed-off-by: Jason C. Leach <[email protected]>

TimoGlastra · 2025-01-18T07:47:52Z

This is the env feature in node 20.6+ https://dev.to/usulpro/nodejs-a-guide-to-native-env-support-and-local-development-300g

Edit: seems it only works if you can node and register ts-node: TypeStrong/ts-node#2058 (comment)

TimoGlastra · 2025-01-18T07:48:48Z

Re the biome rule, somehow it doesn't recognize the service is being used for dependency injection. It shouldn't be type import there. Disabling the rule makes sense for now

sairanjit · 2025-01-19T09:52:28Z

package.json

@@ -18,7 +18,7 @@
    "test": "jest",
    "build": "tsc -p tsconfig.build.json",
    "prepublishOnly": "pnpm build",
-    "dev": "ts-node --env-file=.env.development --env-file .env.local dev",
+    "dev": "ts-node src/index.ts",


@jleach Do you want to add the config related changes to dev.ts as well ? As it helps if we want to run locally using ngrok

I don't think you need dev.ts any more. Just change the AGENT_ENDPOINTS='https://mediator.example.ca,wss://mediator.example.ca' in your .env file and pnpm dev and you're good to go. To me there shouldn't me any more of a change between dev and test. Thoughts?

The dev script connected to ngrok and changed the urls. Now you have to run it manually in another terminal and set the env variables.

It's ok, but does require more setup work

@TimoGlastra I don't mind. I can create create a pnpm dev:standalone or something for ppl that don't use ngrok. Will create a new ticket for tracking since I merged this one already.

jleach added 4 commits January 17, 2025 11:21

jleach requested review from TimoGlastra and sairanjit January 17, 2025 19:50

TimoGlastra reviewed Jan 17, 2025

View reviewed changes

jleach added 4 commits January 17, 2025 13:18

fix: remove node-fetch dep

Verified

This commit was signed with the committer’s verified signature.

jleach Jason C. Leach

SSH Key Fingerprint: 5ZKHntuufDqOZ1zzPmkhbW6QVvXobWbE0/UUbukwF88
Verified
Learn about vigilant mode

8fb38d4

Signed-off-by: Jason C. Leach <[email protected]>

fix: remove code comments

Verified

This commit was signed with the committer’s verified signature.

jleach Jason C. Leach

SSH Key Fingerprint: 5ZKHntuufDqOZ1zzPmkhbW6QVvXobWbE0/UUbukwF88
Verified
Learn about vigilant mode

5763802

Signed-off-by: Jason C. Leach <[email protected]>

fix: remove ref to client code

Verified

This commit was signed with the committer’s verified signature.

jleach Jason C. Leach

SSH Key Fingerprint: 5ZKHntuufDqOZ1zzPmkhbW6QVvXobWbE0/UUbukwF88
Verified
Learn about vigilant mode

f1f355f

Signed-off-by: Jason C. Leach <[email protected]>

jleach force-pushed the fix/di-and-config branch from 22bff80 to f7803d1 Compare January 17, 2025 23:03

jleach marked this pull request as ready for review January 17, 2025 23:12

jleach requested a review from TimoGlastra January 17, 2025 23:12

TimoGlastra approved these changes Jan 18, 2025

View reviewed changes

TimoGlastra linked an issue Jan 18, 2025 that may be closed by this pull request

Add a /health endpoint #13

Closed

sairanjit reviewed Jan 19, 2025

View reviewed changes

jleach merged commit 215ffa7 into openwallet-foundation:main Jan 19, 2025
4 checks passed

jleach deleted the fix/di-and-config branch January 19, 2025 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: di and config mgmt #38

fix: di and config mgmt #38

jleach commented Jan 17, 2025 •

edited

Loading

jleach commented Jan 17, 2025

TimoGlastra left a comment

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

jleach Jan 17, 2025 •

edited

Loading

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

TimoGlastra Jan 17, 2025

sairanjit commented Jan 17, 2025

jleach commented Jan 17, 2025 •

edited

Loading

TimoGlastra commented Jan 18, 2025 •

edited

Loading

TimoGlastra commented Jan 18, 2025

sairanjit Jan 19, 2025

jleach Jan 19, 2025 •

edited

Loading

TimoGlastra Jan 20, 2025

jleach Jan 20, 2025

		messageType: message.messageType,
		clientCode: message.clientCode \|\| '5b4d6bc6-362e-4f53-bdad-ee2742bc0de3',

		token: pushNotificationFcmRecord?.deviceToken \|\| '',
		clientCode: pushNotificationFcmRecord?.clientCode \|\| '',


		console.log(`AGENT_PORT: ${config.get('agent:port')}`)

fix: di and config mgmt #38

fix: di and config mgmt #38

Conversation

jleach commented Jan 17, 2025 • edited Loading

jleach commented Jan 17, 2025

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleach Jan 17, 2025 • edited Loading

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

sairanjit commented Jan 17, 2025

jleach commented Jan 17, 2025 • edited Loading

TimoGlastra commented Jan 18, 2025 • edited Loading

TimoGlastra commented Jan 18, 2025

Choose a reason for hiding this comment

jleach Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleach commented Jan 17, 2025 •

edited

Loading

jleach Jan 17, 2025 •

edited

Loading

jleach commented Jan 17, 2025 •

edited

Loading

TimoGlastra commented Jan 18, 2025 •

edited

Loading

jleach Jan 19, 2025 •

edited

Loading