[DSDK-178] [TS] [SDK] Setup InversifyJS and SOLID conventions #9

valpinkman · 2024-01-23T14:27:23Z

📝 Description

This PR aims to bring the setup for InversifyJS following the SOLID principles.

There is a new internal config module that can be used as an example to implement new modules.
To make it easier there is a module:init script in the core package.json.

From the root

pnpm core module:init moduleName

Explanation of each folder in the config module

data: different data sources and their implementation
service: (or repository) provide business logic, uses the data sources
usecase: used in the public API, uses the services
model: different types used by our domain (here config)
di: where all the binding for the dependency injection is done

Each file is provided with an example of unit test, and there is coverage (only scoped to src/internal for now but we widen it as we progress in the development process

❓ Context

JIRA or GitHub link: DSDK-178

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.

vercel · 2024-01-23T14:27:30Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
device-sdk-ts-sample	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 24, 2024 3:48pm

packages/core/scripts/add-module.mjs

packages/core/src/di.ts

packages/core/src/internal/config/data/LocalConfigDataSource.ts

packages/core/src/internal/config/service/ConfigService.ts

packages/core/src/internal/config/data/RemoteConfigDataSource.ts

smartine-ledger · 2024-01-24T08:04:41Z

packages/core/src/internal/config/model/Config.ts

@@ -0,0 +1,6 @@
+// Domain scoped types
+
+export type Config = {


[ASK] Wrong package. Should be in api/ ?

Discussed with @mbertin-ledger and we decided to put it in the model folder for domain types

IMHO this is no sense as this object is exposed to the app (which is considered as the future client of the SDK). Let's discuss with the team if you prefer 😉

I just see the UseCase is also exposed. In the future I don't think this will be like that. IMHO it misses a component acting as a bridge between the app and the sdk(cf. https://github.com/LedgerHQ/device-sdk-mobile/blob/develop/sdk/src/main/java/com/ledger/devicesdk/sdk/LedgerDeviceSDK.kt)

smartine-ledger

Good first Clean Archi and SOLID implementation. Just a few comments 🙂

smartine-ledger · 2024-01-24T08:08:40Z

packages/core/src/internal/config/data/ConfigDataSource.ts

+export interface LocalConfigDataSource {
+  getConfig(): Config;
+}
+
+export interface RemoteConfigDataSource {


[ASK] 100% personal but I prefer creating 2 distincts files to make the navigation in the project easier

I don't have a strong opinion here, we decided to set it up this way with @mbertin-ledger, but let's ask everyone what they think ?

Same as @smartine-ledger, I prefer 2 files

Same I prefer 2 files

packages/core/src/internal/config/data/LocalConfigDataSource.ts

packages/core/src/internal/config/data/RemoteConfigDataSource.test.ts

smartine-ledger · 2024-01-24T08:12:36Z

packages/core/src/internal/config/di/configModule.ts

+    bind(types.RemoteConfigDataSource).to(RestRemoteConfigDataSource);
+    bind(types.GetSDKVersionUseCase).to(GetSDKVersionUseCase);
+    bind(types.ConfigService).to(DefaultConfigService);


[SHOULD] Single Responsability Principle Violation. Consider creating different files 😉

Not sure what do you mean here, the only responsibility is initialising the ContainerModule.

Right. But all containers should not be initialising in the same file. If you do you are creating a god object.

Again, not use I understand, as here we are only initialising the config module, which is loaded in src/di.ts file

you are not just initialising the config but also the datasources and the usecases. It is not a big deal for the moment but this will in a big app.
I think you will understand my point when we have much more component to initialise 😉

packages/core/src/di.ts

packages/core/src/internal/config/data/LocalConfigDataSource.ts

aussedatlo · 2024-01-24T09:18:50Z

packages/core/src/internal/config/di/configModule.test.ts

+      expect(mod).toBeDefined();
+    });
+
+    it("should return none mocked data sources", () => {


[ASK] What about adding GIVEN WHEN THEN comments like suggested here ?

this is more pseudo language for tests like Cucumber / Gherkin tests. Also, more oriented toward E2E test in my opinion

yeah we need to see how we will integrate Cucumber / Gherkin tests - i agree I don't think it's necessary to have those pseudo language inside unit tests

Already discussed about that with @mbertin-ledger and I disagree adding this kind of comments 👍 . It's too verbose

aussedatlo · 2024-01-24T09:22:42Z

packages/core/src/internal/config/service/DefaultConfigService.ts

+import { inject, injectable } from "inversify";
+import "reflect-metadata";
+import { ConfigService } from "./ConfigService";
+import { types } from "../di/configTypes";


[ASK] How about avoiding relative imports?

I currently don't have the setup for aliases in TS.
However as I said, I was never a big fan on them, but I won't be against it if it's something we agree upon

aussedatlo · 2024-01-24T09:27:39Z

Nice PR overall, just one change requested regarding reflect-metadata imports. 👍

packages/core/src/internal/config/data/DTO.ts

packages/core/src/internal/config/di/configModule.ts

alexandremgo

Great setup PR ! thx 🦖

packages/core/src/internal/config/data/LocalConfigDataSource.stub.ts

ofreyssinet-ledger

great job 🙏

jdabbech-ledger

LGTM great setup 🙌

jdabbech-ledger · 2024-01-24T14:27:20Z

[SHOULD] delete root tests folder as test module files are in use

poc: setup SOLID and inversify

4be158a

valpinkman force-pushed the chore/clean-archi branch from 39d60ca to 4be158a Compare January 23, 2024 14:27

vercel bot deployed to Preview January 23, 2024 14:32 View deployment

jiyuzhuang assigned valpinkman Jan 23, 2024

alexandremgo reviewed Jan 23, 2024

View reviewed changes

packages/core/scripts/add-module.mjs Outdated Show resolved Hide resolved

alexandremgo reviewed Jan 23, 2024

View reviewed changes

packages/core/src/di.ts Outdated Show resolved Hide resolved

smartine-ledger reviewed Jan 24, 2024

View reviewed changes

aussedatlo requested changes Jan 24, 2024

View reviewed changes

vercel bot deployed to Preview January 24, 2024 09:44 View deployment

valpinkman force-pushed the chore/clean-archi branch from 17066cf to a396ebb Compare January 24, 2024 09:44

vercel bot deployed to Preview January 24, 2024 09:45 View deployment

chore: update naming to have better convention + named arguments

2ba743e

valpinkman force-pushed the chore/clean-archi branch from a396ebb to 2ba743e Compare January 24, 2024 09:56

vercel bot deployed to Preview January 24, 2024 09:56 View deployment

alexandremgo reviewed Jan 24, 2024

View reviewed changes

packages/core/src/internal/config/data/DTO.ts Outdated Show resolved Hide resolved

jdabbech-ledger reviewed Jan 24, 2024

View reviewed changes

packages/core/src/internal/config/di/configModule.ts Outdated Show resolved Hide resolved

chore: only import reflect-metadata once

8bd27d7

vercel bot deployed to Preview January 24, 2024 10:23 View deployment

vercel bot deployed to Preview January 24, 2024 10:38 View deployment

alexandremgo approved these changes Jan 24, 2024

View reviewed changes

valpinkman force-pushed the chore/clean-archi branch from ac9d800 to 353e17b Compare January 24, 2024 11:19

vercel bot deployed to Preview January 24, 2024 11:19 View deployment

ofreyssinet-ledger reviewed Jan 24, 2024

View reviewed changes

packages/core/src/internal/config/data/LocalConfigDataSource.stub.ts Outdated Show resolved Hide resolved

valpinkman force-pushed the chore/clean-archi branch from 353e17b to fcb5a41 Compare January 24, 2024 13:08

vercel bot deployed to Preview January 24, 2024 13:09 View deployment

ofreyssinet-ledger approved these changes Jan 24, 2024

View reviewed changes

jdabbech-ledger approved these changes Jan 24, 2024

View reviewed changes

valpinkman force-pushed the chore/clean-archi branch from fcb5a41 to 44c6e39 Compare January 24, 2024 15:30

vercel bot deployed to Preview January 24, 2024 15:31 View deployment

chore: move mocks to .stub.ts files

4f1ead8

valpinkman force-pushed the chore/clean-archi branch from 44c6e39 to 4f1ead8 Compare January 24, 2024 15:48

vercel bot deployed to Preview January 24, 2024 15:49 View deployment

aussedatlo approved these changes Jan 25, 2024

View reviewed changes

gre approved these changes Jan 25, 2024

View reviewed changes

smartine-ledger approved these changes Jan 25, 2024

View reviewed changes

valpinkman merged commit 4138604 into develop Jan 25, 2024
4 checks passed

valpinkman deleted the chore/clean-archi branch January 25, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DSDK-178] [TS] [SDK] Setup InversifyJS and SOLID conventions #9

[DSDK-178] [TS] [SDK] Setup InversifyJS and SOLID conventions #9

valpinkman commented Jan 23, 2024 •

edited by jira bot

Loading

vercel bot commented Jan 23, 2024 •

edited

Loading

smartine-ledger Jan 24, 2024

valpinkman Jan 24, 2024

smartine-ledger Jan 24, 2024 •

edited

Loading

smartine-ledger Jan 24, 2024

smartine-ledger left a comment •

edited

Loading

smartine-ledger Jan 24, 2024

valpinkman Jan 24, 2024

aussedatlo Jan 24, 2024

ofreyssinet-ledger Jan 24, 2024

smartine-ledger Jan 24, 2024

valpinkman Jan 24, 2024

smartine-ledger Jan 24, 2024

valpinkman Jan 24, 2024

smartine-ledger Jan 25, 2024 •

edited

Loading

aussedatlo Jan 24, 2024

valpinkman Jan 24, 2024

alexandremgo Jan 24, 2024

smartine-ledger Jan 24, 2024

aussedatlo Jan 24, 2024

valpinkman Jan 24, 2024

aussedatlo commented Jan 24, 2024

alexandremgo left a comment

ofreyssinet-ledger left a comment

jdabbech-ledger left a comment

jdabbech-ledger commented Jan 24, 2024

[DSDK-178] [TS] [SDK] Setup InversifyJS and SOLID conventions #9

[DSDK-178] [TS] [SDK] Setup InversifyJS and SOLID conventions #9

Conversation

valpinkman commented Jan 23, 2024 • edited by jira bot Loading

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartine-ledger Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartine-ledger left a comment • 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

smartine-ledger Jan 25, 2024 • 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

aussedatlo commented Jan 24, 2024

alexandremgo left a comment

Choose a reason for hiding this comment

ofreyssinet-ledger left a comment

Choose a reason for hiding this comment

jdabbech-ledger left a comment

Choose a reason for hiding this comment

jdabbech-ledger commented Jan 24, 2024

valpinkman commented Jan 23, 2024 •

edited by jira bot

Loading

vercel bot commented Jan 23, 2024 •

edited

Loading

smartine-ledger Jan 24, 2024 •

edited

Loading

smartine-ledger left a comment •

edited

Loading

smartine-ledger Jan 25, 2024 •

edited

Loading