isAnyOf/isAllOf HOFs for simple matchers

[douglas-treadwell]

Implementation for features requested in #771, based on discussion there.

Provides higher order functions for matching actions against many type guards or action creators.

Example usage:

const actionA = createAction<string>('a')
const actionB = createAction<number>('b')
const thunkC = createAsyncThunk<string>('c', () => {
  return 'noop'
})

if (isAnyOf(actionA, actionB, thunkC.fulfilled)(myAction)) {
 // myAction has type-guarded correct type
}

// and can also be used with builder.addMatcher()
const reducer = createReducer(
  initialState,
  (builder) => {
    builder.addMatcher(isAnyOf(actionA, actionB), (state, action) => {
      // reduce
    })
  }
);

Currently implements only core functionality.

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit c838179

https://deploy-preview-788--redux-starter-kit-docs.netlify.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c838179:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration

[markerikson]

Looks nice!

Two minor nitpicky questions:

• Seems like we could probably just put both of these into a matchers.ts file together
• At the risk of trying to de-duplicate things too much: it looks like the only difference here code-wise is the .some vs .every (although I see the type assertions are different). Any way this could be de-duplicated? factory-ized or something, maybe? No big deal if not, just thought I'd check

[douglas-treadwell]

@markerikson

I can definitely put these both in matchers.ts if you'd prefer. I was trying to follow what seemed to be a convention of one exported function per file. Please confirm and I'll update the PR.

About the de-duplication, what I could de-duplicate is the inner...

if (hasMatchFunction(matcher)) {
  return matcher.match(action)
} else {
  return matcher(action)
}

... as a matches function, so that they would look like...

const matches = (matcher: Matcher<any>, action: any) => {
  if (hasMatchFunction(matcher)) {
    return matcher.match(action)
  } else {
    return matcher(action)
  }
}

export function isAnyOf<Matchers extends [Matcher<any>, ...Matcher<any>[]]>(
  ...matchers: Matchers
) {
  return (action: any): action is ActionMatchingAnyOf<Matchers> => {
    return matchers.some((matcher) => matches(matcher, action));
  }
}

export function isAllOf<Matchers extends [Matcher<any>, ...Matcher<any>[]]>(
  ...matchers: Matchers
) {
  return (action: any): action is ActionMatchingAllOf<Matchers> => {
    return matchers.every((matcher) => matches(matcher, action));
  }
}

If I combine them into matchers.ts the helper function matches would be local to both of them.

[douglas-treadwell]

@markerikson

PR is updated. After de-duplicating the code, moving them into the same file made a lot of sense.

[markerikson]

Yeahhhhh, I like the way that looks :)

We haven't had a specific "one function per file" convention - it's just sort of ended up that way because we have some fairly sizeable implementations and types for each of our core APIs.

Obviously this isn't a huge change, but it does save a few bytes in the final output.

[markerikson]

Just out of curiosity, could you use an optional chaining check there instead of a try/catch?

[douglas-treadwell]

Just out of curiosity, could you use an optional chaining check there instead of a try/catch?

That should be fine. I'm guessing you're concerned about potential performance issues?

I tried that, but the build fails with Support for the experimental syntax 'optionalChaining' isn't currently enabled.

I could do the manual equivalent if you prefer. I was erring on the side of being careful about not breaking things.

[douglas-treadwell]

In theory, the original code was perfectly safe. The only arguments that could be passed in (due to type-checking) were either a function, or an object with a match property, so casting and checking the match property should not cause a runtime error. If you like, I can revert the latest commit.

Or, let me know what you think about...

export const hasMatchFunction = <T>(
  v: Matcher<T>
): v is HasMatchFunction<T> => {
  return v && typeof (v as HasMatchFunction<T>).match === 'function'
}

I'm not sure the extent to which the code should be forgiving of mistakes that wouldn't be caught by people using non-TypeScript RTK. If we're being very careful about that, perhaps I should also confirm the else case in the matches function is actually a function, in case isAllOf was called by plain JS with undefined, for example. Or would an exception be acceptable in that case, as a result of user error?

For example, the following (combined with the above) would prevent isAnyOf and isAllOf from throwing exceptions if called improperly from plain JS:

const matches = (matcher: Matcher<any>, action: any) => {
  if (hasMatchFunction(matcher)) {
    return matcher.match(action)
  } else if (typeof matcher === 'function') {
    return matcher(action)
  } else {
    return false;
  }
}

Or do you prefer to let an exception be thrown so they'll recognize their mistake?

[phryneas]

So far, this looks very good. Could you add some type tests as well? (See the separate type-tests folder.

[phryneas]

Yeah, optional chaining is still not an option, since we have tests running back to pre-optional-chaining TS versions.
But I'd say something like

Suggested change

	try {
	return typeof (v as HasMatchFunction<T>).match === 'function'
	} catch {
	return false
	}
	return v && typeof (v as HasMatchFunction<T>).match === 'function'

would totally suffice here.
No need to provoke & catch an exception if it can be prevented in the first place.

[phryneas]

Or do you prefer to let an exception be thrown so they'll recognize their mistake?

I'm usually be all in for an error to be thrown early in development opposed to something like this staying hidden because of some graceful recovery - so I say just let it throw.

[douglas-treadwell]

So far, this looks very good. Could you add some type tests as well? (See the separate type-tests folder.

Of course. Are there any specific cases I should make sure to cover?

I've also replaced the try/catch with the exception prevention you suggested.

[phryneas]

Nothing specific comes to mind - check the type tests for createAction.

One thing: maybe add extra tests that the combined matchers don't widen the type to any. It's a bit tricky to test that, but something along the lines

const actionNotAny: IsAny<typeof action, true, false> = false

should so the trick.
Or just try to access properties that should not be on there and expect an error with // typings:expect-error

Edit: I had this the wrong way round first. Haven't had coffee yet -.-

[douglas-treadwell]

It's been a busy day. I'll be able to add the type tests tomorrow or the day after at the latest.

[douglas-treadwell]

@phryneas @markerikson

I've added some type tests for isAnyOf and isAllOf for the scenarios of: 1) type guards, 2) action creators, and 3) action creators belonging to async thunks.

I'm surprised I haven't heard of typings-tester before. It seems like an incredibly useful tool.

[phryneas]

This looks very good to me.
I'll go ahead and merge this already into a "matchers" branch, so we can track this featureset as a whole and avoid a too big squash commit containing everything in the end :)

Let's continue with the thunk-related stuff, alternative signatures and docs in three seperate PRs. You still up for it? :)

[phryneas]

I'm surprised I haven't heard of typings-tester before. It seems like an incredibly useful tool.

Honestly, there are a lot better tools around, like tsd. But none of them really does all in a nice fashion and switching these around is a chore. So I guess it does the job. And yeah, with a lib of this complexity we need some type-testing lib :)

[douglas-treadwell]

This looks very good to me.
I'll go ahead and merge this already into a "matchers" branch, so we can track this featureset as a whole and avoid a too big squash commit containing everything in the end :)

Let's continue with the thunk-related stuff, alternative signatures and docs in three seperate PRs. You still up for it? :)

Absolutely. I'll start with the extra thunk-related stuff first.

Do you have any thoughts about where these functions should be mentioned in the docs, when I start on that?

[phryneas]

I guess we should add an extra point matchers in the API section and link to that from createReducer, createAsyncThunk in the builders, and add a short paragraph to the "usage with TypeScript" part.

[markerikson]

nice!