Skip to content

fix(core): drop support for plural event/gesture names #10539

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 7, 2024
Merged

fix(core): drop support for plural event/gesture names #10539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2c78a87
chore(core): drop support for plural events
shirakaba May 4, 2024
ba5893c
chore(core): indicate all plural gesture types
shirakaba May 4, 2024
1f976fd
fix(core): drop (the already limited) support for plural gesture events
shirakaba May 4, 2024
ae43960
fix(core): update tests
shirakaba May 5, 2024
08b8cb3
fix(core): identify gestures by exact match
shirakaba May 5, 2024
8cdd4e6
chore(core): remove unused eventNamesRegex
shirakaba May 5, 2024
201af04
fix(core): prevent regression in isGesture()
shirakaba May 5, 2024
c469a40
chore(core): write up thoughts on getEventOrGestureName() and isGestu…
shirakaba May 7, 2024
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
20 changes: 12 additions & 8 deletions apps/automated/src/data/observable-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export var test_Observable_addEventListener_MultipleEvents = function () {
obj.addEventListener(events, callback);
obj.set('testName', 1);
obj.test();
TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange,tested', as we have dropped support for listening to plural event names.");
};

export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function () {
Expand All @@ -176,13 +176,14 @@ export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function

var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
obj.addEventListener(events, callback);
TKUnit.assert(obj.hasListeners(Observable.propertyChangeEvent), 'Observable.addEventListener for multiple events should trim each event name.');
TKUnit.assert(obj.hasListeners(TESTED_NAME), 'Observable.addEventListener for multiple events should trim each event name.');
TKUnit.assert(obj.hasListeners(events), "Expected a listener to be present for event name 'propertyChange , tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), "Expected no listeners to be present for event name 'propertyChange', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(TESTED_NAME), "Expected no listeners to be present for event name 'tested', as we have dropped support for splitting plural event names.");

obj.set('testName', 1);
obj.test();

TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_addEventListener_MultipleCallbacks = function () {
Expand Down Expand Up @@ -223,7 +224,7 @@ export var test_Observable_addEventListener_MultipleCallbacks_MultipleEvents = f
obj.set('testName', 1);
obj.test();

TKUnit.assert(receivedCount === 4, 'The propertyChanged notification should be raised twice.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested' with two different callbacks, as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_removeEventListener_SingleEvent_SingleCallback = function () {
Expand Down Expand Up @@ -341,19 +342,22 @@ export var test_Observable_removeEventListener_MultipleEvents_SingleCallback = f

var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
obj.addEventListener(events, callback);
TKUnit.assert(obj.hasListeners(events), "Expected a listener to be present for event name 'propertyChange , tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), "Expected no listeners to be present for event name 'propertyChange', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(TESTED_NAME), "Expected no listeners to be present for event name 'tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");

obj.set('testName', 1);
obj.test();

obj.removeEventListener(events, callback);

TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), 'Expected result for hasObservers is false');
TKUnit.assert(!obj.hasListeners(TESTED_NAME), 'Expected result for hasObservers is false.');
TKUnit.assert(!obj.hasListeners(events), "Expected the listener for event name 'propertyChange , tested' to have been removed, as we have dropped support for splitting plural event names.");

obj.set('testName', 2);
obj.test();

TKUnit.assert(receivedCount === 2, 'Expected receive count is 2');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_removeEventListener_SingleEvent_NoCallbackSpecified = function () {
Expand Down
136 changes: 62 additions & 74 deletions packages/core/data/observable/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,6 @@ const _globalEventHandlers: {
};
} = {};

const eventNamesRegex = /\s*,\s*/;

/**
* Observable is used when you want to be notified when a change occurs. Use on/off methods to add/remove listener.
* Please note that should you be using the `new Observable({})` constructor, it is **obsolete** since v3.0,
Expand Down Expand Up @@ -153,93 +151,89 @@ export class Observable {

/**
* A basic method signature to hook an event listener (shortcut alias to the addEventListener method).
* @param eventNames - String corresponding to events (e.g. "propertyChange"). Optionally could be used more events separated by `,` (e.g. "propertyChange", "change").
* @param eventName Name of the event to attach to.
* @param callback - Callback function which will be executed when event is raised.
* @param thisArg - An optional parameter which will be used as `this` context for callback execution.
*/
public on(eventNames: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventNames, callback, thisArg);
public on(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventName, callback, thisArg);
}

/**
* Adds one-time listener function for the event named `event`.
* @param event Name of the event to attach to.
* @param eventName Name of the event to attach to.
* @param callback A function to be called when the specified event is raised.
* @param thisArg An optional parameter which when set will be used as "this" in callback method call.
*/
public once(event: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(event, callback, thisArg, true);
public once(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventName, callback, thisArg, true);
}

/**
* Shortcut alias to the removeEventListener method.
*/
public off(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
this.removeEventListener(eventNames, callback, thisArg);
public off(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
this.removeEventListener(eventName, callback, thisArg);
}

/**
* Adds a listener for the specified event name.
* @param eventNames Comma delimited names of the events to attach the listener to.
* @param eventName Name of the event to attach to.
* @param callback A function to be called when some of the specified event(s) is raised.
* @param thisArg An optional parameter which when set will be used as "this" in callback method call.
*/
public addEventListener(eventNames: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
once = once || undefined;
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (typeof callback !== 'function') {
throw new TypeError('Callback, if provided, must be a function.');
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const list = this._getEventList(eventName, true);
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
// Already added.
continue;
}

list.push({
callback,
thisArg,
once,
});
const list = this._getEventList(eventName, true);
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
// Already added.
return;
}

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

/**
* Removes listener(s) for the specified event name.
* @param eventNames Comma delimited names of the events the specified listener is associated with.
* @param eventName Name of the event to attach to.
* @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(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
if (typeof eventName !== 'string') {
throw new TypeError('Events name(s) must be string.');
}

if (callback && typeof callback !== 'function') {
throw new TypeError('callback must be function.');
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const entries = this._observers[eventName];
if (!entries) {
continue;
}
const entries = this._observers[eventName];
if (!entries) {
return;
}

Observable.innerRemoveEventListener(entries, callback, thisArg);
Observable.innerRemoveEventListener(entries, callback, thisArg);

if (!entries.length) {
// Clear all entries of this type
delete this._observers[eventName];
}
if (!entries.length) {
// Clear all entries of this type
delete this._observers[eventName];
}
}

Expand Down Expand Up @@ -297,11 +291,11 @@ export class Observable {
* in future.
* @deprecated
*/
public static removeEventListener(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
public static removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (callback && typeof callback !== 'function') {
Expand All @@ -310,24 +304,22 @@ export class Observable {

const eventClass = this.name === 'Observable' ? '*' : this.name;

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
if (!entries) {
continue;
}
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
if (!entries) {
return;
}

Observable.innerRemoveEventListener(entries, callback, thisArg);
Observable.innerRemoveEventListener(entries, callback, thisArg);

if (!entries.length) {
// Clear all entries of this type
delete _globalEventHandlers[eventClass][eventName];
}
if (!entries.length) {
// Clear all entries of this type
delete _globalEventHandlers[eventClass][eventName];
}

// Clear the primary class grouping if no list are left
const keys = Object.keys(_globalEventHandlers[eventClass]);
if (keys.length === 0) {
delete _globalEventHandlers[eventClass];
}
// Clear the primary class grouping if no list are left
const keys = Object.keys(_globalEventHandlers[eventClass]);
if (keys.length === 0) {
delete _globalEventHandlers[eventClass];
}
}

Expand All @@ -336,12 +328,12 @@ export class Observable {
* in future.
* @deprecated
*/
public static addEventListener(eventNames: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public static addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
once = once || undefined;
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (typeof callback !== 'function') {
Expand All @@ -353,17 +345,15 @@ export class Observable {
_globalEventHandlers[eventClass] = {};
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
if (!_globalEventHandlers[eventClass][eventName]) {
_globalEventHandlers[eventClass][eventName] = [];
}
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
// Already added.
return;
}

_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
if (!_globalEventHandlers[eventClass][eventName]) {
_globalEventHandlers[eventClass][eventName] = [];
}
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
// Already added.
return;
}

_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
}

private _globalNotify<T extends EventData>(eventClass: string, eventType: string, data: T): void {
Expand Down Expand Up @@ -464,10 +454,8 @@ export class Observable {
};
}

public _emit(eventNames: string): void {
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
this.notify({ eventName, object: this });
}
public _emit(eventName: string): void {
this.notify({ eventName, object: this });
}

private _getEventList(eventName: string, createIfNeeded?: boolean): Array<ListenerEntry> | undefined {
Expand Down
29 changes: 25 additions & 4 deletions packages/core/ui/core/bindable/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ViewBase } from '../view-base';
// Requires
import { unsetValue } from '../properties';
import { Observable, PropertyChangeData } from '../../../data/observable';
import { fromString as gestureFromString } from '../../../ui/gestures/gestures-common';
import { addWeakEventListener, removeWeakEventListener } from '../weak-event-listener';
import { bindingConstants, parentsRegex } from '../../builder/binding-builder';
import { escapeRegexSymbols } from '../../../utils';
Expand Down Expand Up @@ -87,25 +88,45 @@ export interface ValueConverter {
toView: (...params: any[]) => any;
}

/**
* Normalizes "ontap" to "tap", and "ondoubletap" to "ondoubletap".
*
* Removes the leading "on" from an event gesture name, for example:
* - "ontap" -> "tap"
* - "ondoubletap" -> "doubletap"
* - "onTap" -> "Tap"
*
* Be warned that, as event/gesture names in NativeScript are case-sensitive,
* this may produce an invalid event/gesture name (i.e. "doubletap" would fail
* to match the "doubleTap" gesture name), and so it is up to the consumer to
* handle the output properly.
*/
export function getEventOrGestureName(name: string): string {
return name.indexOf('on') === 0 ? name.substr(2, name.length - 2) : name;
return name.indexOf('on') === 0 ? name.slice(2) : name;
}

// NOTE: method fromString from "ui/gestures";
export function isGesture(eventOrGestureName: string): boolean {
// Not sure whether this trimming and lowercasing is still needed in practice
// (all Core tests pass without it), so worth revisiting in future. I think
// this is used exclusively by the XML flavour, and my best guess is that
// maybe it's to handle how getEventOrGestureName("onTap") might pass "Tap"
// into this.
const t = eventOrGestureName.trim().toLowerCase();

// Would be nice to have a convenience function for getting all GestureState
// names in `gestures-common.ts`, but when I tried introducing it, it created
// a circular dependency that crashed the automated tests app.
return t === 'tap' || t === 'doubletap' || t === 'pinch' || t === 'pan' || t === 'swipe' || t === 'rotation' || t === 'longpress' || t === 'touch';
}

// TODO: Make this instance function so that we dont need public statc tapEvent = "tap"
// TODO: Make this instance function so that we dont need public static tapEvent = "tap"
// in controls. They will just override this one and provide their own event support.
export function isEventOrGesture(name: string, view: ViewBase): boolean {
if (typeof name === 'string') {
const eventOrGestureName = getEventOrGestureName(name);
const evt = `${eventOrGestureName}Event`;

return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName.toLowerCase());
return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName);
}

return false;
Expand Down
12 changes: 6 additions & 6 deletions packages/core/ui/core/view/view-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,11 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);
const gestureType: GestureTypes | undefined = gestureFromString(normalizedName);

// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
this._observe(gesture, callback, thisArg);
if (gestureType && !this._isEvent(normalizedName)) {
this._observe(gestureType, callback, thisArg);
return;
}

Expand All @@ -324,11 +324,11 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);
const gestureType: GestureTypes | undefined = gestureFromString(normalizedName);

// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
this._disconnectGestureObservers(gesture, callback, thisArg);
if (gestureType && !this._isEvent(normalizedName)) {
this._disconnectGestureObservers(gestureType, callback, thisArg);
return;
}

Expand Down
Loading
Loading