[DSDK-183] [TS] [DSDK] Error/Type safety with purify-ts #12

valpinkman · 2024-01-25T10:39:49Z

chore: add purify-ts in the template module
chore: setup aliases in project

📝 Description

Following the PoC and study about using a potential FP library in the project, here is a rewrite of the module that ship error safety using purify-ts
The interesting part of this is how we can handle easily the different types of error and at which point we do this.

From what I could see during the implementation of this, the Either (aka Result) and Maybe (aka Option) helpers will be mostly used by the data sources as it might not be safe getting
value from these external sources. The Either and Maybe are handled in the services, and the usecase never knows about these types.
Let's see how we manage to tame this tool into the library: it does make things a little more verbose, however the error/type safety is pretty amazing.

❓ Context

JIRA or GitHub link: DSDK-183

✅ Checklist

Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.

Covered by automatic tests.
[ ] Impact of the changes:

🧐 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-25T10:39:52Z

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 29, 2024 9:53am

valpinkman · 2024-01-25T10:44:08Z

⚠️ during this PR, i realised that the test.ts files were included in the ./lib when building it.

I have tried removing them from the tsconfig.json file, doing this however will make eslint bitch about the files being linted but not included in the tsconfig.json. I have not dig into it too much yet, but we definitely will not want the test.ts files in the exported ./lib

edit: FIXED

packages/core/jest.config.ts

packages/core/tsconfig.json

jdabbech-ledger · 2024-01-25T14:21:54Z

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

+const parseResponseSpy = jest.spyOn(
+  RestRemoteConfigDataSource.prototype as any,
+  "_parseResponse"
+);


[ASK] if we spy on a private class methods to check the method calls of RemoteConfigDataSource, does this class respect the single responsibility principle?
Shouldn't it be splitted with a Repository class?
@mbertin-ledger @smartine-ledger

The point is not the SRP but the Unit Test best practice : a private method must not be tested. To be honest I am not sure to understand why we try doing that 👍 🤔

so we can have all the different state during the tests (good json, bad formatted json, bad api call etc...).

In this case this should be tested through RemoteDataSource.getConfig() (not by calling private methods)😉 . A UT tests what is exposed by your classe (= public / internal methods). Nothing more

But the coverage tho....

also we are not testing those private functions, we just make them behave differently so .getConfig() can throw all the possible cases (Error or Success). It would be the same as mocking a fetch or api call.

There is also a test that just run them vanilla so we do go through them "privately".

This way I get into all the different branches of my code and get a 100% coverage on what I've developed

I do not have a good understanding when testing in JS/TS. We may need to discuss about that with the team. During a continous improvment? 🙂

Yeah we should talk about it - I don't think we can reach 100% everywhere. There is probably a layer (the outermost layer in the clean archi) that we won't be able to unit tests without doing some stuff like spying, because what they use is not dependency injected into them.

In the example of RemoteConfigDataSource, we could dependency inject an interface representing the fetch/axios api, so we can mock them without using "spy". But they will probably be cases where it's not practical

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

alexandremgo · 2024-01-29T09:31:55Z

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

+export type RemoteConfigFailure =
+  | ApiCallError
+  | ParseResponseError
+  | JSONParseError;


I don't mind, but so in our architecture/module structure we should put the error types definitions inside the di folder ?

where would you put them ?

Maybe in the model or data folder ? Because they are about the model Config ? But i don't have a strong opinion, i'm asking to know where i should put my errors definitions too ahah

… tsconfig

alexandremgo · 2024-01-29T09:59:59Z

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

-    );
-    return JSON.parse(version) as Config;
+  getConfig(): Either<LocalConfigFailure, Config> {
+    return Either.encase(() => stubFsReadFile())


Oh encase seems super practical

alexandremgo

Great addition, Purify is going to be super useful !

jdabbech-ledger

LGTM

chore: add purify-ts in the template module

443b107

chore: setup aliases in project

a55205c

valpinkman force-pushed the chore/add-purify-ts branch from 883dc5c to a55205c Compare January 25, 2024 10:40

vercel bot deployed to Preview January 25, 2024 10:41 View deployment

valpinkman marked this pull request as ready for review January 25, 2024 10:41

valpinkman marked this pull request as draft January 25, 2024 10:51

chore: add path mapping to jest

fad022a

valpinkman marked this pull request as ready for review January 25, 2024 10:56

vercel bot deployed to Preview January 25, 2024 10:56 View deployment

jdabbech-ledger reviewed Jan 25, 2024

View reviewed changes

packages/core/jest.config.ts Show resolved Hide resolved

packages/core/tsconfig.json Outdated Show resolved Hide resolved

jdabbech-ledger reviewed Jan 25, 2024

View reviewed changes

vercel bot deployed to Preview January 25, 2024 15:38 View deployment

chore: fix paths in bundled js

b40877a

valpinkman force-pushed the chore/add-purify-ts branch from 08a38b4 to b40877a Compare January 25, 2024 15:40

vercel bot deployed to Preview January 25, 2024 15:41 View deployment

chore: fix eslint + tsconfig

b327cb2

vercel bot deployed to Preview January 25, 2024 15:55 View deployment

vercel bot deployed to Preview January 26, 2024 16:32 View deployment

jiyuzhuang assigned valpinkman Jan 29, 2024

alexandremgo reviewed Jan 29, 2024

View reviewed changes

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

alexandremgo reviewed Jan 29, 2024

View reviewed changes

chor: remove fs and path from the project and revert to esnext in sdk…

4623e40

… tsconfig

valpinkman force-pushed the chore/add-purify-ts branch from caed192 to 4623e40 Compare January 29, 2024 09:52

vercel bot deployed to Preview January 29, 2024 09:53 View deployment

alexandremgo reviewed Jan 29, 2024

View reviewed changes

alexandremgo approved these changes Jan 29, 2024

View reviewed changes

jdabbech-ledger approved these changes Jan 29, 2024

View reviewed changes

smartine-ledger approved these changes Jan 29, 2024

View reviewed changes

valpinkman merged commit c79f34a into develop Jan 29, 2024
4 checks passed

valpinkman deleted the chore/add-purify-ts branch January 29, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DSDK-183] [TS] [DSDK] Error/Type safety with purify-ts #12

[DSDK-183] [TS] [DSDK] Error/Type safety with purify-ts #12

valpinkman commented Jan 25, 2024 •

edited by jira bot

Loading

vercel bot commented Jan 25, 2024 •

edited

Loading

valpinkman commented Jan 25, 2024 •

edited

Loading

jdabbech-ledger Jan 25, 2024 •

edited

Loading

smartine-ledger Jan 25, 2024 •

edited

Loading

valpinkman Jan 25, 2024

smartine-ledger Jan 25, 2024 •

edited

Loading

valpinkman Jan 25, 2024 •

edited

Loading

smartine-ledger Jan 26, 2024

valpinkman Jan 26, 2024

alexandremgo Jan 29, 2024 •

edited

Loading

alexandremgo Jan 29, 2024

valpinkman Jan 29, 2024

alexandremgo Jan 29, 2024

alexandremgo Jan 29, 2024

alexandremgo left a comment

jdabbech-ledger left a comment

[DSDK-183] [TS] [DSDK] Error/Type safety with purify-ts #12

[DSDK-183] [TS] [DSDK] Error/Type safety with purify-ts #12

Conversation

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

📝 Description

❓ Context

✅ Checklist

[ ] Impact of the changes:

🧐 Checklist for the PR Reviewers

vercel bot commented Jan 25, 2024 • edited Loading

valpinkman commented Jan 25, 2024 • edited Loading

jdabbech-ledger Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

smartine-ledger Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

valpinkman Jan 25, 2024

Choose a reason for hiding this comment

smartine-ledger Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

valpinkman Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

smartine-ledger Jan 26, 2024

Choose a reason for hiding this comment

valpinkman Jan 26, 2024

Choose a reason for hiding this comment

alexandremgo Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

alexandremgo Jan 29, 2024

Choose a reason for hiding this comment

valpinkman Jan 29, 2024

Choose a reason for hiding this comment

alexandremgo Jan 29, 2024

Choose a reason for hiding this comment

alexandremgo Jan 29, 2024

Choose a reason for hiding this comment

alexandremgo left a comment

Choose a reason for hiding this comment

jdabbech-ledger left a comment

Choose a reason for hiding this comment

valpinkman commented Jan 25, 2024 •

edited by jira bot

Loading

vercel bot commented Jan 25, 2024 •

edited

Loading

valpinkman commented Jan 25, 2024 •

edited

Loading

jdabbech-ledger Jan 25, 2024 •

edited

Loading

smartine-ledger Jan 25, 2024 •

edited

Loading

smartine-ledger Jan 25, 2024 •

edited

Loading

valpinkman Jan 25, 2024 •

edited

Loading

alexandremgo Jan 29, 2024 •

edited

Loading