Issue error or warning on empty-string tags in CreateAsyncThunk

[rickhaws]

Request

Detect when API users use a blank string as the first argument to CreateAsyncThunk(). Issue an error or strong warning.

Example: createAsyncThunk('', async (myDto: myApiDto) => { /* ... */ } // blank first argument

Severity

Because of the following characteristics, the resulting bugs are very hard to track down.

• The error is easy to miss by code reviewers
• The error is not detected by common automated tools
• The bugs are latent
  • An empty-string action tag results in actions like '.pending' and '.fulfilled', which are valid in Redux
  • A single empty-string action can function correctly during many development cycles
  • The effects are not seen until another blank action type is created
  • The two coding errors are likely to be in unrelated areas of the software
• The run-time effects are disconnected from the actions that cause them
  • If a user interaction triggers one blank Redux action type, an unintended second action is also triggered
  • The unintended effect is likely in a completely unrelated part of the program
  • The undesired effect may not be noticed until later in the program's execution

Real-World Scenario

Bug Creation

Here is the scenario where the bug was encountered. In our case, we had a stub action being used as a placeholder. The blank action type was deliberately chosen, which proved to be unwise. Later, another action was created where the developer forgot to add an action type. This may have resulted from copying an existing action, deleting the old action type tag, and forgetting to insert a new action type.

// File 1 (flawed)
const stub = createAsyncThunk('', (itemName: string) => Promise.resolve(itemName));    // blank first argument
export const unImplementedAction1 = stub;
export const unImplementedAction2 = stub;

// File 2 (flawed)
export const updateApiItem = createAsyncThunk('', async (myDto: myApiDto) => { /* ... */ }    // blank first argument

Program Behavior

With the addition of File 2, now any time the stub from File 1 was called, the action from File 2 was also triggered. Working code suddenly started failing. The failure was apparently unrelated to the change added in File 2 that caused the failure.

Bug Fix

The fix was to name both action types:

// File 1 (fixed)
const stub = createAsyncThunk('unused-tag', (itemName: string) => Promise.resolve(itemName));
export const unImplementedAction1 = stub;
export const unImplementedAction2 = stub;

// File 2 (fixed)
export const updateApiItem = createAsyncThunk('myApi/put', async (myDto: myApiDto) => { /* ... */ }

[EskiMojo14]

the issue you're reporting is basically just "if i use the same prefix for multiple async thunks, my reducers don't know the difference", which isn't unique to an empty string. you could have two async thunks with the prefix "fetchThing" and encounter the same issue.

[phryneas]

What @EskiMojo14 says:

const a = createAsyncThunk('', () => ... )
const b = createAsyncThunk('', () => ... )

and

const a = createAsyncThunk('foo', () => ... )
const b = createAsyncThunk('foo', () => ... )

are the same bug, there is nothing inherently specific to the empty string here. cAT should not be called twice with the same argument if it doesn't describe the exact same action, which is extremely rare.

That said, I want to pick up one thing you said:

The error is easy to miss by code reviewers

In any code review, no matter if you have deep knowledge of a library or see it used for the first time, you should always be extremely suspicious if you see an empty string passed as a mandatory argument.
If this string were not important, it would be the second or third argument and could be omitted. The fact that it is the first argument is a strong signal from the API designer that this string is extremely important, should never be omitted, and should probably given more thought than "pass in an empty string here, it will be okay".
Any reviewer that encounters an empty string passed in as a mandatory argument should raise an eyebrow and look up the documentation if such use is really recommended.
This is a very red flag and should be extremely easy to spot.