Skip to content

Commit

Permalink
fix(effects): register functional effects from object without prototy…
Browse files Browse the repository at this point in the history
…pe (#3984)

Closes #3972
  • Loading branch information
markostanimirovic authored Aug 4, 2023
1 parent caa74ff commit 1879cc9
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 7 deletions.
34 changes: 31 additions & 3 deletions modules/effects/spec/effect_sources.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ describe('EffectSources', () => {
this.effectIdentifier = identifier;
}
}

class SourceWithInitAction implements OnInitEffects, OnIdentifyEffects {
effectIdentifier: string;

Expand Down Expand Up @@ -207,6 +208,17 @@ describe('EffectSources', () => {
const recordB = {
b: createEffect(() => alwaysOf(b), { functional: true }),
};
// a record with functional effects that is defined as
// a named import in a built package doesn't have a prototype
// for more info see: https://github.com/ngrx/platform/issues/3972
const recordC = Object.freeze({
__proto__: null,
c: createEffect(() => alwaysOf(c), { functional: true }),
});
const recordD = Object.freeze({
__proto__: null,
d: createEffect(() => alwaysOf(d as any), { functional: true }),
});

it('should resolve effects from class instances', () => {
const sources$ = cold('--a--b--', {
Expand All @@ -221,8 +233,12 @@ describe('EffectSources', () => {
});

it('should resolve effects from records', () => {
const sources$ = cold('--a--b--', { a: recordA, b: recordB });
const expected = cold('--a--b--', { a, b });
const sources$ = cold('--a--b--c--', {
a: recordA,
b: recordB,
c: recordC,
});
const expected = cold('--a--b--c--', { a, b, c });

const output = toActions(sources$);

Expand Down Expand Up @@ -338,7 +354,7 @@ describe('EffectSources', () => {
expect(output).toBeObservable(expected);
});

it('should report an error if an effect dispatches an invalid action', () => {
it('should report an error if a class-based effect dispatches an invalid action', () => {
const sources$ = of(new SourceD());

toActions(sources$).subscribe();
Expand All @@ -350,6 +366,18 @@ describe('EffectSources', () => {
);
});

it('should report an error if a functional effect dispatches an invalid action', () => {
const sources$ = of(recordD);

toActions(sources$).subscribe();

expect(mockErrorReporter.handleError).toHaveBeenCalledWith(
new Error(
'Effect "d()" dispatched an invalid action: {"not":"a valid action"}'
)
);
});

it('should report an error if an effect dispatches an `undefined`', () => {
const sources$ = of(new SourceE());

Expand Down
9 changes: 9 additions & 0 deletions modules/effects/spec/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ describe('isClassInstance', () => {
it('returns false for a function', () => {
expect(isClassInstance(() => {})).toBe(false);
});

it('returns false for an object without prototype', () => {
const obj = Object.freeze({
__proto__: null,
foo: 'bar',
});

expect(isClassInstance(obj)).toBe(false);
});
});

describe('isClass', () => {
Expand Down
7 changes: 5 additions & 2 deletions modules/effects/src/effect_notification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { ObservableNotification } from './utils';
export interface EffectNotification {
effect: Observable<any> | (() => Observable<any>);
propertyName: PropertyKey;
sourceName: string;
sourceName: string | null;
sourceInstance: any;
notification: ObservableNotification<Action | null | undefined>;
}
Expand Down Expand Up @@ -46,8 +46,11 @@ function getEffectName({
sourceName,
}: EffectNotification) {
const isMethod = typeof sourceInstance[propertyName] === 'function';
const isClassBasedEffect = !!sourceName;

return `"${sourceName}.${String(propertyName)}${isMethod ? '()' : ''}"`;
return isClassBasedEffect
? `"${sourceName}.${String(propertyName)}${isMethod ? '()' : ''}"`
: `"${String(propertyName)}()"`;
}

function stringify(action: Action | null | undefined) {
Expand Down
4 changes: 3 additions & 1 deletion modules/effects/src/effects_resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ export function mergeEffects(
globalErrorHandler: ErrorHandler,
effectsErrorHandler: EffectsErrorHandler
): Observable<EffectNotification> {
const sourceName = getSourceForInstance(sourceInstance).constructor.name;
const source = getSourceForInstance(sourceInstance);
const isClassBasedEffect = !!source && source.constructor.name !== 'Object';
const sourceName = isClassBasedEffect ? source.constructor.name : null;

const observables$: Observable<any>[] = getSourceMetadata(sourceInstance).map(
({
Expand Down
4 changes: 3 additions & 1 deletion modules/effects/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ export function getSourceForInstance<T>(instance: T): T {

export function isClassInstance(obj: object): boolean {
return (
obj.constructor.name !== 'Object' && obj.constructor.name !== 'Function'
!!obj.constructor &&
obj.constructor.name !== 'Object' &&
obj.constructor.name !== 'Function'
);
}

Expand Down

0 comments on commit 1879cc9

Please sign in to comment.