You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(observable): return this from on, once and off - #11617
Observable.on, once and off returned void, so listeners could not be chained the way Node's EventEmitter allows. They now return the Observable instance (this), so calls like observable.on('tap', handler).once('load', other).off('tap', handler) work.
The same change applies to addEventListener and removeEventListener, including the overrides in ViewBase, ScrollViewBase, MediaQueryList and Span, and the typed on/once/off overloads declared across the core package (including the handwritten .d.ts files) were updated to return this as well, so the public typings match the runtime behavior.
What is the current behavior?
on, once and off return nothing, so chaining listener registrations is not possible.
What is the new behavior?
All of them return the instance they were called on. Existing code that ignores the return value is unaffected.
Tests
New returning this for chaining tests in packages/core/data/observable/index.spec.ts: on/once/off/addEventListener/removeEventListener return the observable, and a chained on().on().once().off() sequence still notifies the remaining listeners. The 5 new tests fail on the clean tree and pass with this change.
Full @nativescript/core unit suite: 56 files, 648 passed, 1 skipped.
tsc --noEmit on the lib config is clean (the spec config has 25 pre-existing errors, unchanged by this change), ESLint on the core package passes, and Prettier is clean on the changed files (3 pre-existing Prettier warnings in unrelated .d.ts files remain untouched).
Observable.on, once and off returned void, so listeners could not be
chained. They now return the Observable instance, matching the Node
EventEmitter API. addEventListener and removeEventListener return this
as well, and the typed on/once/off overloads across the core package
were updated to match.
FixesNativeScript#6828
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Event-listener APIs across core observables, windows, and UI components now return their instance, with Observable tests covering chaining. The Android fullscreen configuration also reapplies edge-to-edge settings when night mode changes.
Changes: Event Listener Return Contracts
Layer / File(s)
Summary
Observable listener methods and tests packages/core/data/observable/*, packages/core/data/observable-array/index.ts, packages/core/data/virtual-array/index.ts, packages/core/media-query-list/index.ts
Observable listener methods now return the instance on registration and removal paths. ObservableArray, VirtualArray, and MediaQueryList listener APIs declare instance return types. Tests check return values and verify chaining with regular and once-listeners.
View, ScrollView, and Span listener methods return their instance after handling listeners. DialogFragmentImpl tracks fullscreen night mode and reapplies edge-to-edge configuration when that mode changes.
Event overloads for UI components and Cache now return this. Button, SearchBar, and WebView native property declarations also move semicolons before inline comments; their types remain unchanged.
Severity of issue fixed: Low
Merge Risk: Low · up to 309fb
Most listener APIs now support chaining, but SharedTransition listener chains still fail and global matchMedia chains remain unavailable to TypeScript callers. These are bounded API gaps; address them and the Span declaration requirement before considering the feature complete.
Pre-merge checks: 4 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Out of Scope Changes check
[#6828] The listener, type, and test changes support chaining. The summary also describes a fullscreen modal night-mode change in packages/core/ui/core/view/index.android.ts, which is unrelated to c…
Provide the merge-base version or whole-PR diff for packages/core/ui/core/view/index.android.ts to establish whether the night-mode code is part of this PR.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title identifies the main change: chainable Observable listener methods.
Description check
The description explains the chainable listener changes and related tests.
Linked Issues check
[#6828] Observable.on, once, and off now return this. The listener overrides and public overloads support the same contract. The new tests check return values and chained listener behavior.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 33 files.
Full details: Out of Scope Changes check
Explanation
[#6828] The listener, type, and test changes support chaining. The summary also describes a fullscreen modal night-mode change in packages/core/ui/core/view/index.android.ts, which is unrelated to chaining. The head contains that code, but the summary reports only six added lines in the file. The base blob and file diff were unavailable, so the evidence does not establish whether the PR introduced the night-mode code.
Resolution
Provide the merge-base version or whole-PR diff for packages/core/ui/core/view/index.android.ts to establish whether the night-mode code is part of this PR.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ESLint
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
ESLint install timed out. The project may have too many dependencies for the sandbox.
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Note
Follow-up findings are listed below so you can review them separately from inline comments.
Follow-ups (2)
Minor (2)
Minor · Annotate every View listener overload with this. · index.d.ts:859
packages/core/ui/core/view/index.d.ts:859 Minor · Functional Correctness · Quick win
Annotate every View listener overload with this.
View.on('loaded', ...) and View.off(...) currently resolve to declarations with implicit any returns. Chained calls can therefore bypass View type checking. Add : this to the generic and event-specific on overloads and to off.
Minor · Update the Span declaration. · span.ts:116
packages/core/ui/text-base/span.ts:116 Minor · Maintainability & Code Quality · Low value
Update the Span declaration.
Span inherits the updated signatures, but span.d.ts is unchanged. Declare the updated listener signatures there.
As per coding guidelines, “update the neighboring .d.ts whenever a public API changes.”
The new tests cover only Observable. They do not execute ViewCommon.addEventListener or ViewCommon.removeEventListener through the gesture branch. A regression to return; would therefore pass the current tests while breaking direct gesture calls such as view.addEventListener('tap', handler).removeEventListener('tap', handler).
Add return-value assertions for duplicate registration and removal of an absent event. These paths are not covered by the new tests, so a bare early return could regress the fluent contract unnoticed.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
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.
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 branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Fixes #6828
Observable.on,onceandoffreturnedvoid, so listeners could not be chained the way Node'sEventEmitterallows. They now return theObservableinstance (this), so calls likeobservable.on('tap', handler).once('load', other).off('tap', handler)work.The same change applies to
addEventListenerandremoveEventListener, including the overrides inViewBase,ScrollViewBase,MediaQueryListandSpan, and the typedon/once/offoverloads declared across the core package (including the handwritten.d.tsfiles) were updated to returnthisas well, so the public typings match the runtime behavior.What is the current behavior?
on,onceandoffreturn nothing, so chaining listener registrations is not possible.What is the new behavior?
All of them return the instance they were called on. Existing code that ignores the return value is unaffected.
Tests
returning this for chainingtests inpackages/core/data/observable/index.spec.ts:on/once/off/addEventListener/removeEventListenerreturn the observable, and a chainedon().on().once().off()sequence still notifies the remaining listeners. The 5 new tests fail on the clean tree and pass with this change.@nativescript/coreunit suite: 56 files, 648 passed, 1 skipped.tsc --noEmiton the lib config is clean (the spec config has 25 pre-existing errors, unchanged by this change), ESLint on the core package passes, and Prettier is clean on the changed files (3 pre-existing Prettier warnings in unrelated.d.tsfiles remain untouched).