fix(ability): ensure ability call all event handlers during emit

Event when they are unsunscribed in emit phase

packages/casl-ability/spec/ability.spec.js

@@ -157,7 +157,7 @@ describe('Ability', () => {
       expect(ability).not.to.allow('publish', 'Post')
     })
 
-    describe('`update` method', () => {
+    describe('`update` method and events', () => {
       let updateHandler
 
       beforeEach(() => {
@@ -201,24 +201,46 @@ describe('Ability', () => {
         expect(anotherHandler).to.have.been.called()
       })
 
-      it('invokes all subscribers even if they are changed during "emit" phase', () => {
-        const firstSubscription = setupListenerChangesInListener()
-        const secondSubscription = setupListenerChangesInListener()
+      it('can unregister itself inside event handler', () => {
+        const handlers = [spy(), spy()]
+        const unsubscribe1 = ability.on('updated', () => {
+          handlers[0]()
+          unsubscribe1()
+        })
+        ability.on('updated', handlers[1])
 
         ability.update([])
 
-        expect(firstSubscription).to.have.been.called()
-        expect(secondSubscription).to.have.been.called()
-      })
+        expect(handlers[0]).to.have.been.called()
+        expect(handlers[1]).to.have.been.called()
+      })
+
+      it('can unregister another event handler inside own handler', () => {
+        let results = []
+        const handlers = [
+          spy(() => results.push(0)),
+          spy(() => results.push(1)),
+          spy(() => results.push(2)),
+          spy(() => results.push(3))
+        ]
+        const unsubscribe = []
+
+        unsubscribe[0] = ability.on('updated', handlers[0])
+        unsubscribe[1] = ability.on('updated', handlers[1])
+        unsubscribe[2] = ability.on('updated', handlers[2])
+        unsubscribe[3] = ability.on('updated', () => {
+          handlers[3]()
+          unsubscribe[2]()
+        })
+        ability.update([])
+
+        expect(results).to.deep.equal([3, 2, 1, 0])
 
-      function setupListenerChangesInListener() {
-        const unsubscribe = spy(ability.on('update', function listen() {
-          unsubscribe()
-          ability.on('update', listen)
-        }))
+        results = []
+        ability.update([{ action: 'read', subject: 'all' }])
 
-        return unsubscribe
-      }
+        expect(results).to.deep.equal([3, 1, 0])
+      })
     })
   })
 

packages/casl-ability/src/RuleIndex.ts

@@ -9,7 +9,7 @@ import {
   ExtractSubjectType
 } from './types';
 import { wrapArray, detectSubjectType, mergePrioritized, getOrDefault, identity, isSubjectType } from './utils';
-import { LinkedItem, linkedItem, unlinkItem } from './structures/LinkedItem';
+import { LinkedItem, linkedItem, unlinkItem, cloneLinkedItem } from './structures/LinkedItem';
 
 export interface RuleIndexOptions<A extends Abilities, C> extends Partial<RuleOptions<C>> {
   detectSubjectType?(
@@ -48,16 +48,20 @@ interface AbilityEvent<T extends WithGenerics> {
 export interface UpdateEvent<T extends WithGenerics> extends AbilityEvent<T> {
   rules: RawRuleOf<T>[]
 }
+/**
+ * @deprecated `on`/`emit` properly infer type without this type
+ * TODO(major): delete
+ */
 export type EventHandler<Event> = (event: Event) => void;
 
 export type Events<
   T extends WithGenerics,
   K extends keyof EventsMap<T> = keyof EventsMap<T>
-> = Map<K, LinkedItem<EventHandler<EventsMap<T>[K]>> | null>;
+> = Map<K, LinkedItem<EventsMap<T>[K]> | null>;
 
 interface EventsMap<T extends WithGenerics> {
-  update: UpdateEvent<T>
-  updated: UpdateEvent<T>
+  update(event: UpdateEvent<T>): void
+  updated(event: UpdateEvent<T>): void
 }
 
 type IndexTree<A extends Abilities, C> = Map<SubjectType, Map<string, {
@@ -212,7 +216,7 @@ export class RuleIndex<A extends Abilities, Conditions> {
 
   on<T extends keyof EventsMap<this>>(
     event: T,
-    handler: EventHandler<EventsMap<Public<this>>[T]>
+    handler: EventsMap<Public<this>>[T]
   ): Unsubscribe {
     const head = this._events.get(event) || null;
     const item = linkedItem(handler, head);
@@ -227,10 +231,13 @@ export class RuleIndex<A extends Abilities, Conditions> {
     };
   }
 
-  private _emit<T extends keyof EventsMap<this>>(name: T, payload: EventsMap<this>[T]) {
+  private _emit<T extends keyof EventsMap<this>>(
+    name: T,
+    payload: Parameters<EventsMap<this>[T]>[0]
+  ) {
     let current = this._events.get(name) || null;
     while (current !== null) {
-      const prev = current.prev;
+      const prev = current.prev ? cloneLinkedItem(current.prev) : null;
       current.value(payload);
       current = prev;
     }

packages/casl-ability/src/structures/LinkedItem.ts

@@ -4,17 +4,17 @@ export interface LinkedItem<T> {
   readonly value: T
 }
 
-export const linkedItem = <T>(value: T, prev: LinkedItem<T>['prev']) => {
+export function linkedItem<T>(value: T, prev: LinkedItem<T>['prev']) {
   const item = { value, prev, next: null };
 
   if (prev) {
     prev.next = item;
   }
 
   return item;
-};
+}
 
-export const unlinkItem = (item: LinkedItem<any>) => {
+export function unlinkItem(item: LinkedItem<any>) {
   if (item.next) {
     item.next.prev = item.prev;
   }
@@ -24,4 +24,10 @@ export const unlinkItem = (item: LinkedItem<any>) => {
   }
 
   item.next = item.prev = null; // eslint-disable-line
-};
+}
+
+export const cloneLinkedItem = <T extends LinkedItem<any>>(item: T): T => ({
+  value: item.value,
+  prev: item.prev,
+  next: item.next,
+} as T);