Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add match function to Json module #1522

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m-bock
Copy link
Contributor

@m-bock m-bock commented Jun 25, 2021

I think this may be useful to destructure Json data structures.

#1519

@samhh
Copy link
Contributor

samhh commented Jun 25, 2021

This is broader than this PR but is there a reason we favour pattern matching in the form of match(f, g) instead of match({ f, g })?

I've had comments from colleagues I was onboarding to fp-ts previously that match/fold is hard to understand because you don't know which callback aligns with which member of the union or sum type. It's manageable for types with fewer members like Option where you can memorise the order, but for a case like this it's going to be very difficult. In a sense, where the types overlap, we're also giving up type safety with the current approach.

Here's a comparison in terms of readability (keeping in mind you'd usually not have the type referenced in the return value!):

J.match(
  () => 'null',
  () => 'boolean',
  () => 'number',
  () => 'string',
  () => 'array',
  () => 'object'
)

J.match({
  onNull: () => 'null',
  onBoolean: () => 'boolean',
  onNumber: () => 'number',
  onString: () => 'string',
  onArray: () => 'array',
  onObject: () => 'object'
})

An additional benefit is that this lets you order the callbacks however you'd like. We could additionally experiment with how to support a universal _/otherwise fallback.

@DenisFrezzato
Copy link
Collaborator

is there a reason we favour pattern matching in the form of match(f, g) instead of match({ f, g })?

I guess for verbosity's sake. I'm fine with match(f, g) for well known members and as long as there are a couple of cases. For custom unions I usually go with an object where the keys are the name of the members. Example of usage:

type Err = { _type: 'NotFound', id: string } | { _type: 'SomeException', error: Error }

match({
  NotFound: ({ id }) => `Resource not found with ID "${id}"`,
  SomeException: ({ error }) => error.message
})

(I actually use ts-adt for pattern matching, which follows this idea).

src/Json.ts Outdated Show resolved Hide resolved
@m-bock
Copy link
Contributor Author

m-bock commented Jun 25, 2021

This is broader than this PR but is there a reason we favour pattern matching in the form of match(f, g) instead of match({ f, g })?

Personally I'd favor the {f, g...} approach, too. I just used the positional one to stay in sync with other match functions.
In my view it's not even a matter of the number of cases, even for something simple like Either I find {left, right} safer.

_/'otherwise' cases I would not handle at this level. But I think it would make sense to add isString, isNumber (...) predicates for each case. (like it's done in other modules)

@m-bock
Copy link
Contributor Author

m-bock commented Jun 25, 2021

Btw, I guess you have noticed the as assertion needed in the last case. I tried this approach:

export const match: <Z>(
    onNull: () => Z,
    onBool: (x: boolean) => Z,
    onNum: (x: number) => Z,
    onStr: (x: string) => Z,
    onArr: (x: JsonArray) => Z,
    onObj: (x: JsonRecord) => Z
  ) => (j: Json) => Z = (onNull, onBool, onNum, onStr, onArr, onObj) => (j) =>
    j === null
      ? onNull()
      : typeof j === 'boolean'
      ? onBool(j)
      : typeof j === 'number'
      ? onNum(j)
      : typeof j === 'string'
      ? onStr(j)
      : j instanceof Array
      ? onArr(j)
      : onObj(j)

But unfortunately this did not pass the typelevel tests for TS version 3.5 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants