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

feat: Refactor ActionWithPayload/ActionWithoutPayload to be uniform #605

Merged
merged 9 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"react": "18.2.0",
"react-test-renderer": "18.2.0",
"rimraf": "3.0.2",
"rxjs": "7.5.7",
"rxjs": "7.8.0",
"rxjs-marbles": "7.0.1",
"standard-version": "9.5.0",
"typescript": "4.9.3"
Expand Down
6 changes: 3 additions & 3 deletions src/action$.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { incrementMocks } from './internal/testing/mock';
import { action$, dispatchAction } from './action$';
import { take } from 'rxjs/operators';
import { _namespaceAction } from './namespace';
import { ActionWithoutPayload } from './types/Action';
import { UnknownAction } from './internal';

const actions = incrementMocks.marbles.actions;

Expand All @@ -13,7 +13,7 @@ afterEach(() => {
});

test('dispatchAction makes action$ emit', () => {
let lastAction: ActionWithoutPayload | undefined;
let lastAction: UnknownAction | undefined;
const sub = action$.pipe(take(1)).subscribe((a) => (lastAction = a));
cleanupFns.push(() => sub.unsubscribe());

Expand All @@ -22,7 +22,7 @@ test('dispatchAction makes action$ emit', () => {
});

test('dispatchAction does not remove existing namespace', () => {
let lastAction: ActionWithoutPayload | undefined;
let lastAction: UnknownAction | undefined;
const sub = action$.pipe(take(1)).subscribe((a) => (lastAction = a));
cleanupFns.push(() => sub.unsubscribe());

Expand Down
5 changes: 3 additions & 2 deletions src/actionCreator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import {
isActionOfType,
isValidRxBeachAction,
} from './actionCreator';
import { ActionName } from './types/Action';

type Payload = { num: number };
const myAction = actionCreator<Payload>('[test] three');
const action = myAction({ num: 3 }) as {
type: string;
type: ActionName;
payload: Payload;
meta: { namespace?: string };
};
Expand All @@ -32,7 +33,7 @@ test('actionCreator should create action objects with the payload', () => {
});
test('actionCreator should create action objects with protected type', () => {
expect(() => {
action.type = 'mock';
action.type = '[test] mock';
}).toThrow(TypeError);
});
test('actionCreator should create action objects with protected meta', () => {
Expand Down
20 changes: 12 additions & 8 deletions src/actionCreator.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import type { Action, VoidPayload } from './types/Action';
import type { ActionCreator, ActionCreatorFunc } from './types/ActionCreator';
import type { Action, ActionName, VoidPayload } from './types/Action';
import type { ActionCreator } from './types/ActionCreator';

/**
* Untyped `actionCreator`
* Create an action creator
*
* **You code should not hit this untyped overload**
* If you see this message in your IDE, you should investigate why TS did not
* recognize the generic, typed overload of this function.
* @param type A name for debugging purposes
* @template `Payload` - The payload type for the action
* @returns An action creator function that accepts a payload as input, and
* returns a complete action object with that payload and a type unique
* to this action creator
*/
export const actionCreator: ActionCreatorFunc = (type: string) => {
const actionCreatorFn = (payload?: any) =>
export const actionCreator = <Payload = void>(
type: ActionName
): ActionCreator<Payload> => {
const actionCreatorFn = (payload: Payload) =>
Object.freeze({
type,
payload,
Expand Down
25 changes: 23 additions & 2 deletions src/actionCreator.tspec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,27 @@ for (const [creatorFn, anAction] of actionPairs) {
const action1 = actionCreatorWithoutPayload();
if (isActionOfType(actionCreatorWithoutPayload, action1)) {
// @ts-expect-error Assert that ActionWithoutPayload does not have a payload
// eslint-disable-next-line no-unused-expressions
action1.payload;
isNonNullish(action1.payload);
}

// eslint-disable-next-line @typescript-eslint/ban-types
const isNonNullish = (x: {}) => {
/* no-op */
};

const testIsActionOfType = (action: any) => {
if (!isValidRxBeachAction(action)) {
return;
}

if (isActionOfType(actionCreatorWithPayload, action)) {
return isNonNullish(action.payload);
}

if (isActionOfType(actionCreatorWithoutPayload, action)) {
// @ts-expect-error should not have payload
return isNonNullish(action.payload);
}

isNonNullish(action.type);
};
4 changes: 2 additions & 2 deletions src/internal/testing/utils.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Action } from '../../types/Action';
import type { Action, ActionName } from '../../types/Action';
import { VoidPayload } from '../types';

export const mockAction = <P = VoidPayload>(
type: string,
type: ActionName,
namespace?: string,
payload?: P
): Action<P> =>
Expand Down
6 changes: 3 additions & 3 deletions src/internal/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { OperatorFunction } from 'rxjs';
import { ActionWithPayload, ActionWithoutPayload } from '../types/Action';
import { Action, ActionName, ActionWithPayload } from '../types/Action';

export { VoidPayload } from '../types/Action';

Expand All @@ -10,10 +10,10 @@ export { VoidPayload } from '../types/Action';
* want to extract a possible `payload` from an action you don't know anything
* about.
*/
export type UnknownAction = ActionWithoutPayload & { payload?: unknown };
export type UnknownAction = Action<unknown>;

export interface ActionCreatorCommon {
readonly type: string;
readonly type: ActionName;
}

/**
Expand Down
6 changes: 0 additions & 6 deletions src/internal/types.tspec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { AssertFalse, Has } from 'conditional-type-checks';
import { ActionWithPayload, ActionWithoutPayload } from '../types/Action';
import {
ActionCreatorWithPayload,
Expand All @@ -24,11 +23,6 @@ let unknownActionCreator: UnknownActionCreator;
unknownAction = actionWithoutPayload;
unknownAction = actionWithPayload;

type ActionCreatorWithoutPayload_is_not_assibleable_to_UnknownActionCreatorWithPayload =
AssertFalse<
Has<ActionCreatorWithoutPayload, UnknownActionCreatorWithPayload<unknown>>
>;

Comment on lines -27 to -31
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case did not make sense.

// ActionCreatorWithPayload and UnkownActionCreatorWithPayload is assignable to each other
unknownActionCreatorWithPayload = actionCreatorWithPayload;
actionCreatorWithPayload = unknownActionCreatorWithPayload;
Expand Down
11 changes: 6 additions & 5 deletions src/namespace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@ import { ActionDispatcher } from './types/helpers';
import { UnknownAction } from './internal/types';
import { _namespaceAction } from './namespace';
import { mockAction } from './internal/testing/utils';
import { ActionName } from './types/Action';

const namespaced = _namespaceAction('namespace', mockAction('type')) as {
type: string;
const namespaced = _namespaceAction('namespace', mockAction('[Mock] type')) as {
type: ActionName;
meta: {
namespace: string;
};
};
test('_namespaceAction has unwritable type', () => {
expect(() => {
namespaced.type = 'foo';
namespaced.type = '[Mock] foo';
}).toThrow(TypeError);
});

Expand All @@ -29,7 +30,7 @@ test('_namespaceAction has unwritable namespace', () => {
});

test('namespaceActionCreator should create actions with namespace', () => {
const type = 'action type';
const type: ActionName = '[Mock] action type';
const namespace = 'new namespace';
const actionCreator = (payload: number) =>
mockAction(type, 'old namespace', payload);
Expand All @@ -55,7 +56,7 @@ test('namespaceActionDispatcher should invoke the parent dispatcher with namespa
parentDispatcher
);

const actionObject = mockAction('action', 'old namespace');
const actionObject = mockAction('[Mock] action', 'old namespace');

childDispatcher(actionObject);

Expand Down
2 changes: 1 addition & 1 deletion src/operators/operators.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ test.each`
({ payload }: { name: string; payload: unknown }) => {
marbles((m) => {
const source = m.hot<ActionWithPayload<any>>('aa', {
a: mockAction('', '', payload) as ActionWithPayload<any>,
a: mockAction('[Mock] a', '', payload),
});
const expected = m.hot('pp', {
p: payload,
Expand Down
2 changes: 1 addition & 1 deletion src/persistentDerivedStream.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ObservableInput } from 'rxjs';
import type { ObservableInput } from 'rxjs';
import { ObservableState } from './observableState';
import { stateStreamRegistry } from './stateStreamRegistry';

Expand Down
78 changes: 45 additions & 33 deletions src/routines.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Subject } from 'rxjs';
import { filter, map } from 'rxjs/operators';
import { marbles } from 'rxjs-marbles/jest';
import { ActionWithPayload } from './types/Action';
import { Action } from './types/Action';
import {
Routine,
collectRoutines,
Expand All @@ -18,55 +18,67 @@ beforeAll(() => {
jest.useFakeTimers();
});

const actions = {
a: mockAction('alpha', undefined, { letter: 'A' }),
b: mockAction('bravo', undefined, { letter: 'B' }),
c: mockAction('charlie', undefined, { letter: 'C' }),
e: mockAction('error'),
const marbleMap: Record<string, any> = {
a: mockAction('[Mock] alpha', undefined, { letter: 'A' }),
b: mockAction('[Mock] bravo', undefined, { letter: 'B' }),
c: mockAction('[Mock] charlie', undefined, { letter: 'C' }),
e: mockAction('[Mock] error'),
f: mockAction('[Mock] action', undefined, { letter: 'F' }),
};
const letters = {
A: 'A',
B: 'B',
C: 'C',
F: 'F',
};
const lengths = {
...letters,
'5': 5,
'7': 7,
};

// Add letter representations to marble map
for (let i = 'A'.charCodeAt(0); i < 'Z'.charCodeAt(0); i++) {
const letter = String.fromCharCode(i);
marbleMap[letter] = letter;
}
// Add number representations to marble map
for (let i = 0; i < 10; i++) {
// Create marble representations of the 10 first characters
// eg. { A: 'A', B: 'B', ...etc }
const charCodeA = 'A'.charCodeAt(0);
const letter = String.fromCharCode(charCodeA + i);
marbleMap[letter] = letter;
// Create marble representations of the 10 first numbers from 0
marbleMap[`${i}`] = i;
}
Comment on lines +29 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change these tests because it was using the lengths of the action types.. which obviously changes when you enforce the ActionName type.
Now the tests are more robust.


const errors = {
e: 'error',
};
const actionMarbles1 = ' -a----b----c---';
const letterMarbles = ' -A----B----C---';
const combinedMarbles = '-(A5)-(B5)-(C7)';
const combinedMarbles = '-(A0)-(B1)-(C2)';
const actionMarbles2 = ' -a----e----a---';
const errorMarbles = ' ------e';
const errorSub1 = ' ^-----!';
const errorSub2 = ' ------^--------';
const singleActionMarble = ' -f---f';

const errorRoutine: Routine<string> = map((a) => {
if (a.type === 'error') throw 'error';
if (a.type === '[Mock] error') throw 'error';
return 'passed';
});
const lettersRoutine = routine(
filter(
(action: any): action is ActionWithPayload<{ letter: string }> =>
action.payload !== undefined
),
filter((action: unknown): action is Action<{ letter: string }> => {
const { payload } = action as Action<{ letter: string }>;
return Boolean(payload.letter);
}),
extractPayload(),
map(({ letter }) => letter)
);
const lengthRoutine = routine(map(({ type }) => type.length));

const letterToNumberRoutine = routine(
map((action) => {
const { payload } = action as Action<{ letter: string }>;
return payload.letter.charCodeAt(0) - 'A'.charCodeAt(0);
})
);

test(
'routine pipes multiple operator functions',
marbles((m) => {
const action$ = m.hot(actionMarbles1, actions);
const expected$ = m.hot(letterMarbles, letters);
const action$ = m.hot(actionMarbles1, marbleMap);
const expected$ = m.hot(letterMarbles, marbleMap);

const actual$ = action$.pipe(lettersRoutine);

Expand All @@ -77,10 +89,10 @@ test(
test(
'collectRoutines runs all routines',
marbles((m) => {
const action$ = m.hot(actionMarbles1, actions);
const expected$ = m.hot(combinedMarbles, lengths);
const action$ = m.hot(actionMarbles1, marbleMap);
const expected$ = m.hot(combinedMarbles, marbleMap);
const actual$ = action$.pipe(
collectRoutines(lettersRoutine, lengthRoutine)
collectRoutines(lettersRoutine, letterToNumberRoutine)
);
m.expect(actual$).toBeObservable(expected$);
})
Expand All @@ -89,7 +101,7 @@ test(
test(
'subscribeRoutine subscribes action$',
marbles((m) => {
const action$ = m.hot(actionMarbles1, actions);
const action$ = m.hot(actionMarbles1, marbleMap);
subscribeRoutine(action$, lettersRoutine);

m.expect(action$).toHaveSubscriptions(['^']);
Expand All @@ -99,7 +111,7 @@ test(
test(
'subscribeRoutine resubscribes on errors',
marbles((m) => {
const action$ = m.hot(actionMarbles2, actions);
const action$ = m.hot(actionMarbles2, marbleMap);

subscribeRoutine(action$, errorRoutine);

Expand All @@ -110,7 +122,7 @@ test(
test(
'subscribeRoutine emits errors to error subject',
marbles((m) => {
const action$ = m.hot(actionMarbles2, actions);
const action$ = m.hot(actionMarbles2, marbleMap);
const error$ = new Subject<any>();

subscribeRoutine(action$, errorRoutine, error$);
Expand All @@ -122,7 +134,7 @@ test(
test(
'tapRoutine register a routine',
marbles((m) => {
const action$ = m.hot(singleActionMarble, actions);
const action$ = m.hot(singleActionMarble, marbleMap);
const action = actionCreator<{ letter: string }>('[Mock] action');
expect.assertions(2);
const routineToSubscribe = tapRoutine(action, (payload) =>
Expand Down
Loading