refactor: Redefine UsernameMapperEntry with io-ts - [merged] #134

Closed famedly-bot closed 1 year ago

In GitLab by @agraven on Dec 20, 2022, 16:59

Merges agraven/usermapper-io-ts -> main

This is an alternative take on !92 that uses the io-ts package to achieve type safe deserialization instead of the more manual approach.

In GitLab by @agraven on Dec 20, 2022, 16:59

requested review from @jbecker, @kate-shine, @ghost, @jakibaki, @ghost, @ghost, @jcgruenhage, and @jdreichmann

In GitLab by @agraven on Dec 21, 2022, 07:38

added 1 commit

6ca83a2f - refactor: Redefine UsernameMapperEntry with io-ts

Compare with previous version

In GitLab by @agraven on Dec 21, 2022, 09:30

added 1 commit

ec6a490a - refactor: Redefine UsernameMapperEntry with io-ts

Compare with previous version

In GitLab by @ghost on Dec 22, 2022, 07:58

Commented on src/usernamemapper.ts line 111

This one feels a bit rusty, maybe maintainability might be increased by using a more TypeScript-y implementation here?

In GitLab by @agraven on Dec 22, 2022, 14:28

Commented on src/usernamemapper.ts line 111

The alternative approaches would be either the "fp-ts way" using its pipe utility for chaining functions

return pipe(
    // parse the JSON
    Json.decode(entry.toString()),
    // Deserialize it to a UsernameMapperEntry
    E.chain((obj) => UsernameMapperEntry.decode(obj)),
    // Eval to null if any errors occurred
    E.getOrElseW(() => null),
)

Or to copy-paste fp.ts's definition in place, which feels needlessly wordy to me:

const parsed = JSON.parse(entry.toString());
const result = UsernameMapperEntry.decode(parsed);
if (E.isRight(result)) {
    return result.right
} else {
    throw new Error(PathReporter.report(result).join(" "))
}

Neither feel too appealing to me, since the first strikes me as even less familiar to people used to the standard typescript paradigm, and the second doesn't really help readability in my view.

In GitLab by @kate-shine on Dec 22, 2022, 16:39

Commented on src/usernamemapper.ts line 111

There's a question if the issue doesn't stem from the fact that fp-ts itself quite changes the paradigm.

On the other hand, I'd say that being more Rust-y is not a bad thing for current BE development and given the fact that UIA will be phased out at (hopefully) near future, it's an ok thing to do.

In GitLab by @kate-shine on Dec 23, 2022, 13:08

approved this merge request

In GitLab by @agraven on Dec 23, 2022, 13:10

added 2 commits

cf4b7145 - 1 commit from branch main
e5e7061a - refactor: Redefine UsernameMapperEntry with io-ts

Compare with previous version

In GitLab by @agraven on Dec 23, 2022, 13:10

enabled an automatic merge when the pipeline for e5e7061a1a8bae57cf37541618db6a80c98206cf succeeds

famedly / uia-proxy

refactor: Redefine UsernameMapperEntry with io-ts - [merged] #134