remove dependencies on node specific libraries#135
remove dependencies on node specific libraries#135jakubkoci merged 2 commits intoopenwallet-foundation:masterfrom
Conversation
Signed-off-by: Timo Glastra <timo@animo.id>
when run in node environment the native events pacakge will be used. in other environments the npm package will be used Signed-off-by: Timo Glastra <timo@animo.id>
jakubkoci
left a comment
jakubkoci
left a comment
There was a problem hiding this comment.
Good job Timo, I added just two notes, but I'm approving this.
| */ | ||
| public static fromString(string: string) { | ||
| return JSON.parse(string); | ||
| } |
There was a problem hiding this comment.
I'm worried if this is not too much abstraction. I would suggest just calling JSON.parse directly.
There was a problem hiding this comment.
Maybe. We're already using the JSON Encoder for all the other stuff so I thought it will be consistent in a way that everything that has to do with JSON encoding is done by the JsonEncoder.
I'm fine with removing it
| */ | ||
| public static toString(json: unknown) { | ||
| return JSON.stringify(json); | ||
| } |
There was a problem hiding this comment.
I'm worried if this is not too much abstraction. I would suggest just calling JSON.stringify directly.
There was a problem hiding this comment.
Same as with fromString, I'm fine with removing it
| */ | ||
| public static fromBuffer(buffer: Buffer) { | ||
| return JsonEncoder.fromString(buffer.toString('utf-8')); | ||
| } |
There was a problem hiding this comment.
Tricky question: Should this be part of the JsonEncoder or BufferEncoder? 😄
There was a problem hiding this comment.
Yeah I wasn't sure either. But splitting toBuffer and fromBuffer between JsonEncoder and BufferEncoder also doesn't seem like the best approach to me. What do you think?
2 participants
We were using three NodeJS specific libraries:
base64encoding with a smallbase64ToBase64URLutil function.eventsdependency. In NodeJS the nativeeventslibrary will be used. In RN (or other platforms) the npm package will be used (Node will only use the npm package if you importevents/instead ofeventsso we're good here)bufferdependency and use an import fromutil/buffer.tsinstead of global. Same as with events the native library will be used in NodeJS and the npm package in other environments