Make invalid usage of createFetchStore actions result in Jest test failures as expected

[felixarntz]

Bug Description

With the function signature changes in #1707, we missed quite a few occurrences where these were called but not updated (see #1799), which was fortunately caught before release. The alarming thing is that none of our Jest tests had failed because of that.

A part of that is due to createFetchStore not being strict enough. When calling the fetch* action with invalid arguments, it just uses console.error which will not cause our Jest tests to fail by default. The reason to use console errors was taken only because invariant wouldn't actually throw errors (which was even the case before switching to createFetchStore). It seems that the way @wordpress/data works we cannot throw errors from those generator functions?

An alternative is that we should possibly change our approach for Jest tests so that any unexpected console.error causes a test failure. This may be something we need to do anyway, see #1770.

Either way, we should find a way so that such invalid usages make our tests fail.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• createFetchStore should receive a new argument called validateParams. The function should receive params and throw on failure (typically via invariant). The default should just be an empty function.
• Documentation for argsToParams should be adjusted to no longer include validation.
• params should be validated with validateParams for:
  • determining whether the receiver requires params
  • the fetch action
  • the receive action (this is where it's new, main purpose of this issue)
• All createFetchStore instances that currently include validation in argsToParams should move that out of there and instead provide an appropriate validateParams function.

Implementation Brief

• Add a new optional parameter validateParams, which defaults to () => {}
  • Add an invariant call to ensure validateParams is always a function
• Update the logic for determining if requiresParams to call validateParams in the try/catch
• Call validateParams( params ) in the fetchCreator action
  • Remove wrapping try/catch and console.error calls
• Call validateParams( params ) in the receiveCreator action, after the invariant call for "params is required"
  • Remove wrapping try/catch and console.error calls
• Call validateParams( params ) in the isFetching selector, inside the try block
• Update fetchCreator action to be a normal non-generator function
  This is the main reason why validation is not handled as expected
  • Extract the generator to a new internal function which is invoked (passing { args, params }) and returned by the fetchCreator
    Only params is currently needed, but args will be needed by changes coming in Implement more appropriate error handling in JS data stores and refactored components #1814 so we can pass both, even though only one is currently needed.
• Update documentation for args.argsToParams to only be passed if params are needed
• Add documentation for new args.validateParams
• Update existing createFetchStore usage to move invariant calls into the new validateParams function arg
  Currently 20 instances
• Finish and merge Fix createFetchStore validation #1874
  Mostly just needs existing usage updated

QA Brief

• This is a refactoring with no observable change in behavior – things should continue to work the same as before
• Special attention should be paid to actions which trigger fetch requests from the datastore as this was the focus of the changes

Changelog entry

• Fix internal error handling so that invalid usages of API-based selectors result in errors being thrown as expected.

[aaemnnosttv]

The function should receive params and return it on success, or throw on failure (typically via invariant). The default should just pass through params.

@felixarntz regarding this part, I don't think it's necessary that the validation function returns anything, it only needs to throw if something is invalid. I've written the IB accordingly - unless there's another reason why this is needed, I can update it accordingly.

[felixarntz]

@aaemnnosttv Mostly LGTM, however one thing to correct I think:

• Change the logic for determining if requiresParams to use argsToParams without a try/catch
  • requiresParams = argsToParams !== defaultArgsToParams
    i.e. if argsToParams is passed a function, it means it requires params, it shouldn't be passed otherwise

I think this should instead call argsToParams and then validateParams in the try/catch. If an error is thrown here, then requiresParams should be true. This is closer to how it currently works, the user passing an argsToParams doesn't mean that these are required, now that validation happens separately.

[aaemnnosttv]

Just wanted to leave this finding here of a similar issue upstream: WordPress/gutenberg#17771

[felixarntz]

@aaemnnosttv

Extract the generator to a new internal function which is invoked (passing params) and returned by the fetchCreator

This sounds great to finally fix that issue, however we'll need to also still pass args to that internal action, to use them e.g. for #1814. Let's just pass both params and args. Since this bit is fully internal, it's okay if that's not super intuitive, we should just document why both are being passed.

After that change the IB should be good to go from my end.

[aaemnnosttv]

@felixarntz I've updated the IB to pass both to the internal fetchCreator function. Alternatively, we could just pass ...args as it would have been called before and call argsToParams internally and not call validateParams again as they've already been validated. It would be kind of an unnecessary call since we already have the params but a bit more consistent. Not a big deal either way, but I've updated it like you said 🙂

[felixarntz]

IB ✅

[ryanwelcher]

Everything looks to be functioning as expected
QA ✔️