Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions packages/core/data/observable-array/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ export interface ObservableArray<T> {
* `this` context when the callback is called. Falsy values will be not be
* bound.
*/
on(eventName: string, callback: (data: EventData) => void, thisArg?: any): void;
on(eventName: string, callback: (data: EventData) => void, thisArg?: any): this;

on(event: 'change', callback: (args: ChangedData<T>) => void, thisArg?: any): void;
on(event: 'change', callback: (args: ChangedData<T>) => void, thisArg?: any): this;
}
38 changes: 38 additions & 0 deletions packages/core/data/observable/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,44 @@ describe('Observable', () => {
});
});

describe('returning this for chaining', () => {
it('on returns the observable', () => {
const observable = new Observable();
expect(observable.on('test', () => {})).toBe(observable);
});

it('once returns the observable', () => {
const observable = new Observable();
expect(observable.once('test', () => {})).toBe(observable);
});

it('off returns the observable', () => {
const observable = new Observable();
const handler = () => {};
observable.on('test', handler);
expect(observable.off('test', handler)).toBe(observable);
});

it('addEventListener and removeEventListener return the observable', () => {
const observable = new Observable();
const handler = () => {};
expect(observable.addEventListener('test', handler)).toBe(observable);
expect(observable.removeEventListener('test', handler)).toBe(observable);
});

it('chains on, once and off and still notifies the remaining listeners', () => {
const observable = new Observable();
const calls: string[] = [];
const first = () => calls.push('first');
const second = () => calls.push('second');
const third = () => calls.push('third');
observable.on('test', first).on('test', second).once('test', third).off('test', first);
observable.notify({ eventName: 'test', object: observable });
observable.notify({ eventName: 'test', object: observable });
expect(calls).toEqual(['second', 'third', 'second']);
});
});

describe('hasListeners', () => {
it('reflects listener registration and removal', () => {
const observable = new Observable();
Expand Down
24 changes: 17 additions & 7 deletions packages/core/data/observable/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,10 @@ export class Observable {
* `this` context when the callback is called. Falsy values will be not be
* bound.
*/
public on(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
public on(eventName: string, callback: (data: EventData) => void, thisArg?: any): this {
this.addEventListener(eventName, callback, thisArg);

return this;
Comment on lines +189 to +192

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

rg -n --glob '*.ts' --glob '*.js' '(on|once|off|addEventListener|removeEventListener)\s*\(' packages/core | head -n 220

Repository: NativeScript/NativeScript

Length of output: 25903


Script executed:

git diff --stat 07e84ec588e1103bda0f58bbb37594bf48ccba57 309fbaa167e2b3e49391e7652039118cb30db01a
nl -ba packages/core/data/observable/index.ts | sed -n '125,310p'
rg -n --glob '*.{ts,js}' '(^|[[:space:]])(public |protected |private |override |static |async )*(on|once|off|addEventListener|removeEventListener)[[:space:]]*\(' packages --glob '!*.spec.ts' --glob '!*.test.ts'

Repository: NativeScript/NativeScript

Length of output: 41344


Script executed:

for f in \
packages/core/data/observable-array/index.ts \
packages/core/data/virtual-array/index.ts \
packages/core/media-query-list/index.ts \
packages/core/native-window/window-base.ts \
packages/core/native-window/native-window-common.ts \
packages/core/ui/core/view/view-common.ts \
packages/core/ui/core/view/index.android.ts \
packages/core/ui/scroll-view/scroll-view-common.ts \
packages/core/ui/text-base/span.ts \
packages/core/ui/shared-transition/shared-transition.ts \
packages/core/ui/transition/shared-transition.ts \
packages/core/ui/image-cache/image-cache-common.ts \
packages/core/ui/page/page-common.ts \
packages/core/ui/web-view/web-view-common.ts \
packages/core/ui/list-view/list-view-common.ts \
packages/core/ui/tab-view/tab-view-common.ts \
packages/core/ui/segmented-bar/segmented-bar-common.ts \
packages/core/ui/placeholder/index.ts \
packages/core/ui/placeholder/index.android.ts; do
  if test -f "$f"; then
    echo "===== $f ====="
    rg -n -C 8 '(^|[[:space:]])(public |protected |private |override |static |async )*(on|once|off|addEventListener|removeEventListener)[[:space:]]*\(' "$f" || true
  fi
done
echo "===== class inheritance ====="
rg -n --glob '*.ts' 'class [A-Za-z0-9_$]+ extends (Observable|ViewBase|ScrollViewBase|NativeWindow|WindowBase|MediaQueryListImpl|Span)' packages/core || true
echo "===== shared transition ====="
nl -ba packages/core/ui/transition/shared-transition.ts | sed -n '170,220p'

Repository: NativeScript/NativeScript

Length of output: 29876


Script executed:

for f in \
packages/core/data/observable-array/index.ts \
packages/core/data/virtual-array/index.ts \
packages/core/media-query-list/index.ts \
packages/core/native-window/window-base.ts \
packages/core/native-window/native-window-common.ts \
packages/core/ui/core/view/view-common.ts \
packages/core/ui/core/view/index.android.ts \
packages/core/ui/scroll-view/scroll-view-common.ts \
packages/core/ui/text-base/span.ts \
packages/core/ui/transition/shared-transition.ts \
packages/core/ui/image-cache/image-cache-common.ts \
packages/core/ui/page/page-common.ts \
packages/core/ui/web-view/web-view-common.ts \
packages/core/ui/list-view/list-view-common.ts \
packages/core/ui/tab-view/tab-view-common.ts \
packages/core/ui/segmented-bar/segmented-bar-common.ts \
packages/core/ui/placeholder/index.ts \
packages/core/ui/placeholder/index.android.ts; do
  if test -f "$f"; then
    echo "===== $f ====="
    rg -n -C 8 '(^|[[:space:]])(public |protected |private |override |static |async )*(on|once|off|addEventListener|removeEventListener)[[:space:]]*\(' "$f" || true
  fi
done
echo "===== class inheritance ====="
rg -n --glob '*.ts' 'class [A-Za-z0-9_$]+ extends (Observable|ViewBase|ScrollViewBase|NativeWindow|WindowBase|MediaQueryListImpl|Span)' packages/core || true
echo "===== shared transition ====="
nl -ba packages/core/ui/transition/shared-transition.ts | sed -n '170,220p'

Repository: NativeScript/NativeScript

Length of output: 29876


Script executed:

rg -n -C 5 -e 'sharedTransitionEvents' -e 'SharedTransitionObservable' packages/core/ui/transition packages/core
rg -n -C 3 -e 'SharedTransition.events' -e 'SharedTransition.get' packages/core apps --glob '*.{ts,js,xml}' || test "$?" -eq 1
nl -ba packages/core/ui/transition/shared-transition.ts | sed -n '215,360p'
nl -ba packages/core/ui/core/view/view-common.ts | sed -n '388,455p'
nl -ba packages/core/ui/core/view/index.android.ts | sed -n '638,680p'
nl -ba packages/core/media-query-list/index.ts | sed -n '110,158p'
nl -ba packages/core/ui/scroll-view/scroll-view-common.ts | sed -n '33,68p'
git diff 07e84ec588e1103bda0f58bbb37594bf48ccba57 309fbaa167e2b3e49391e7652039118cb30db01a -- packages/core/ui/transition/shared-transition.ts

Repository: NativeScript/NativeScript

Length of output: 44157


Return the receiver from SharedTransitionObservable.on.

SharedTransition.events().on(...) returns undefined because the override discards the superclass result. A JavaScript chain then throws, and TypeScript callers cannot chain through on. Return super.on(eventNames, <any>callback, thisArg) from the override.

}

/**
Expand All @@ -201,8 +203,10 @@ export class Observable {
* `this` context when the callback is called. Falsy values will be not be
* bound.
*/
public once(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
public once(eventName: string, callback: (data: EventData) => void, thisArg?: any): this {
this.addEventListener(eventName, callback, thisArg, true);

return this;
}

/**
Expand All @@ -214,8 +218,10 @@ export class Observable {
* @param thisArg An optional parameter which, when set, will be used to
* refine search of the correct event listener to be removed.
*/
public off(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
public off(eventName: string, callback?: (data: EventData) => void, thisArg?: any): this {
this.removeEventListener(eventName, callback, thisArg);

return this;
}

/**
Expand All @@ -225,7 +231,7 @@ export class Observable {
* @param thisArg An optional parameter which when set will be used as "this" in callback method call.
* @param once An optional parameter which when set will cause the event listener to fire once.
*/
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): this {
once = once || undefined;
thisArg = thisArg || undefined;

Expand All @@ -240,14 +246,16 @@ export class Observable {
const list = this._getEventList(eventName, true);
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
// Already added.
return;
return this;
}

list.push({
callback,
thisArg,
once,
});

return this;
}

/**
Expand All @@ -256,7 +264,7 @@ export class Observable {
* @param callback An optional parameter pointing to a specific listener. If not defined, all listeners for the event names will be removed.
* @param thisArg An optional parameter which when set will be used to refine search of the correct callback which will be removed as event listener.
*/
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): this {
thisArg = thisArg || undefined;

if (typeof eventName !== 'string') {
Expand All @@ -269,7 +277,7 @@ export class Observable {

const entries = this._observers[eventName];
if (!entries) {
return;
return this;
}

Observable.innerRemoveEventListener(entries, callback, thisArg);
Expand All @@ -278,6 +286,8 @@ export class Observable {
// Clear all entries of this type
delete this._observers[eventName];
}

return this;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/core/data/virtual-array/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,13 @@ export interface VirtualArray<T> {
* `this` context when the callback is called. Falsy values will be not be
* bound.
*/
on(eventName: string, callback: (data: EventData) => void, thisArg?: any): void;
on(eventName: string, callback: (data: EventData) => void, thisArg?: any): this;
/**
* Raised when still not loaded items are requested.
*/
on(event: 'itemsLoading', callback: (args: ItemsLoading) => void, thisArg?: any): void;
on(event: 'itemsLoading', callback: (args: ItemsLoading) => void, thisArg?: any): this;
/**
* Raised when a change occurs.
*/
on(event: 'change', callback: (args: ChangedData<T>) => void, thisArg?: any): void;
on(event: 'change', callback: (args: ChangedData<T>) => void, thisArg?: any): this;
}
8 changes: 6 additions & 2 deletions packages/core/media-query-list/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class MediaQueryListImpl extends Observable implements MediaQueryList {
}

// @ts-ignore
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): this {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

sed -n '1,75p' packages/core/media-query-list/index.ts; sed -n '200,360p' packages/core/media-query-list/index.ts; rg -n --glob '*.ts' 'interface MediaQueryList|declare.*MediaQueryList|function matchMedia|matchMedia\s*[:=]' packages/core | head -n 70

Repository: NativeScript/NativeScript

Length of output: 3003


Script executed:

git diff 07e84ec588e1103bda0f58bbb37594bf48ccba57 309fbaa167e2b3e49391e7652039118cb30db01a -- packages/core/media-query-list/index.ts packages/core/native-window/index.d.ts packages/core/native-window/window-base.ts packages/core/native-window/native-window-common.ts
nl -ba packages/core/media-query-list/index.ts | sed -n '45,215p'
rg -n -F -e 'matchMedia' -e 'MediaQueryList' -- packages/core/native-window packages/core/globals packages/core/media-query-list packages/core/package.json || test "$?" -eq 1
rg --files packages/core/media-query-list

Repository: NativeScript/NativeScript

Length of output: 41775


Script executed:

nl -ba packages/core/globals/index.ts | sed -n '300,340p'
rg -n -F -e 'interface MediaQueryList' -e 'declare var matchMedia' -e 'matchMedia(' -e 'MediaQueryList' --glob '*.ts' --glob '*.d.ts' --glob 'tsconfig*.json' .
rg -n -F -e '"dom"' -e "'dom'" -e 'lib' --glob 'tsconfig*.json' --glob '*.json' packages/core | head -n 100
find packages/core -maxdepth 3 -type f \( -name '*.d.ts' -o -name 'tsconfig*.json' \) -print

Repository: NativeScript/NativeScript

Length of output: 13555


Script executed:

nl -ba packages/core/tsconfig.lib.json
nl -ba tsconfig.base.json | sed -n '1,140p'
nl -ba apps/ui/tsconfig.json
nl -ba apps/ui/src/media-queries/main-page.ts | sed -n '1,48p'
nl -ba packages/core/package.json | sed -n '1,100p'
nl -ba packages/core/globals/index.ts | sed -n '1,60p;275,330p'
rg -n '"typescript"|typescript@|typescript:' package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -n 50
command -v tsc
TSC="$(command -v tsc)"; if [ -n "$TSC" ]; then ROOT="$(dirname "$(dirname "$(readlink -f "$TSC")")")"; echo "typescript root: $ROOT"; rg -n '^(interface MediaQueryList extends|declare function matchMedia|    addEventListener<K extends keyof MediaQueryListEventMap>|    removeEventListener<K extends keyof MediaQueryListEventMap>)' "$ROOT/lib/lib.dom.d.ts"; fi

Repository: NativeScript/NativeScript

Length of output: 12930


Web query:

TypeScript 5.9.3 lib.dom.d.ts "declare function matchMedia" "interface MediaQueryList" addEventListener void

Result:

Sources consulted:
- https://raw.githubusercontent.com/microsoft/TypeScript/v5.9.3/lib/lib.dom.d.ts
- https://raw.githubusercontent.com/microsoft/TypeScript/v5.9.3/src/lib/dom.generated.d.ts
- Releases · microsoft/TypeScript · GitHub — https://github.com/microsoft/TypeScript/releases
- …and 26 more

Script executed:

readlink -f /usr/local/bin/tsc
ls -l /usr/local/bin/tsc /usr/local/lib/node_modules/typescript 2>&1
nl -ba packages/core/tsconfig.json
nl -ba package.json | sed -n '100,115p'

Repository: NativeScript/NativeScript

Length of output: 1702


Script executed:

TSROOT=/usr/local/lib/node_modules/typescript
rg -n -F -e 'interface MediaQueryList' -e 'declare function matchMedia' -e 'addEventListener<K extends keyof MediaQueryListEventMap>' -e 'removeEventListener<K extends keyof MediaQueryListEventMap>' "$TSROOT/lib/lib.dom.d.ts"
rg -n -F -e 'matchMedia' -e 'MediaQueryList' packages/core/index.ts packages/core/index.d.ts packages/core/global-types.d.ts packages/core/references.d.ts packages/core/typings packages/core 2>/dev/null | head -n 120
nl -ba packages/core/index.ts | sed -n '1,180p'
nl -ba packages/core/index.d.ts | sed -n '1,180p'
nl -ba packages/core/global-types.d.ts | sed -n '1,160p'

Repository: NativeScript/NativeScript

Length of output: 21053


Web query:

site:raw.githubusercontent.com/microsoft/TypeScript/v5.9.3/lib/lib.dom.d.ts "interface MediaQueryList" "addEventListener" "removeEventListener"

Result:

Sources consulted:
- https://raw.githubusercontent.com/microsoft/TypeScript/v5.9.3/lib/lib.dom.d.ts

Expose fluent listener types for the global matchMedia API.

tsconfig.base.json includes the DOM declarations, where MediaQueryList.addEventListener and removeEventListener return void. NativeScript does not augment these declarations, so chaining global matchMedia(...).addEventListener(...) still fails type-checking despite the runtime implementation returning this.

this._throwInvocationError?.();

const hasChangeListeners = this.hasListeners(MediaQueryListImpl.changeEvent);
Expand All @@ -124,10 +124,12 @@ class MediaQueryListImpl extends Observable implements MediaQueryList {
toggleApplicationEventListeners(true, onDeviceChange);
}
}

return this;
}

// @ts-ignore
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): this {
this._throwInvocationError?.();

// Call super method first since it throws in the case of bad parameters
Expand All @@ -147,6 +149,8 @@ class MediaQueryListImpl extends Observable implements MediaQueryList {
}
}
}

return this;
}

addListener(callback: (this: MediaQueryList, ev: MediaQueryListEvent) => any): void {
Expand Down
Loading
Loading