8000 fix(core): drop support for plural event/gesture names (#10539) · apburgess/NativeScript@99d9d39 · GitHub
[go: up one dir, main page]
Skip to content

Commit 99d9d39

Browse files
shirakabaapburgess
shirakaba
authored and
apburgess
committed
fix(core): drop support for plural event/gesture names (NativeScript#10539)
1 parent d88fa48 commit 99d9d39

File tree

8 files changed

+323
-269
lines changed

8 files changed

+323
-269
lines changed

‎apps/automated/src/data/observable-tests.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ export var test_Observable_addEventListener_MultipleEvents = function () {
163163
obj.addEventListener(events, callback);
164164
obj.set('testName', 1);
165165
obj.test();
166-
TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
166+
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.");
167167
};
168168

169169
export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function () {
@@ -176,13 +176,14 @@ export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function
176176

177177
var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
178178
obj.addEventListener(events, callback);
179-
TKUnit.assert(obj.hasListeners(Observable.propertyChangeEvent), 'Observable.addEventListener for multiple events should trim each event name.');
180-
TKUnit.assert(obj.hasListeners(TESTED_NAME), 'Observable.addEventListener for multiple events should trim each event name.');
179+
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.");
180+
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.");
181+
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.");
181182

182183
obj.set('testName', 1);
183184
obj.test();
184185

185-
TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
186+
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).");
186187
};
187188

188189
export var test_Observable_addEventListener_MultipleCallbacks = function () {
@@ -223,7 +224,7 @@ export var test_Observable_addEventListener_MultipleCallbacks_MultipleEvents = f
223224
obj.set('testName', 1);
224225
obj.test();
225226

226-
TKUnit.assert(receivedCount === 4, 'The propertyChanged notification should be raised twice.');
227+
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).");
227228
};
228229

229230
export var test_Observable_removeEventListener_SingleEvent_SingleCallback = function () {
@@ -341,19 +342,22 @@ export var test_Observable_removeEventListener_MultipleEvents_SingleCallback = f
341342

342343
var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
343344
obj.addEventListener(events, callback);
345+
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.");
346+
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.");
347+
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.");
348+
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).");
344349

345350
obj.set('testName', 1);
346351
obj.test();
347352

348353
obj.removeEventListener(events, callback);
349354

350-
TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), 'Expected result for hasObservers is false');
351-
TKUnit.assert(!obj.hasListeners(TESTED_NAME), 'Expected result for hasObservers is false.');
355+
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.");
352356

353357
obj.set('testName', 2);
354358
obj.test();
355359

356-
TKUnit.assert(receivedCount === 2, 'Expected receive count is 2');
360+
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).");
357361
};
358362

359363
export var test_Observable_removeEventListener_SingleEvent_NoCallbackSpecified = function () {

‎packages/core/data/observable/index.ts

Lines changed: 62 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,6 @@ const _globalEventHandlers: {
8989
};
9090
} = {};
9191

92-
const eventNamesRegex = /\s*,\s*/;
93-
9492
/**
9593
* Observable is used when you want to be notified when a change occurs. Use on/off methods to add/remove listener.
9694
* Please note that should you be using the `new Observable({})` constructor, it is **obsolete** since v3.0,
@@ -153,93 +151,89 @@ export class Observable {
153151

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

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

174172
/**
175173
* Shortcut alias to the removeEventListener method.
176174
*/
177-
public off(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
178-
this.removeEventListener(eventNames, callback, thisArg);
175+
public off(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
176+
this.removeEventListener(eventName, callback, thisArg);
179177
}
180178

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

191-
if (typeof eventNames !== 'string') {
192-
throw new TypeError('Event name(s) must be a string.');
189+
if (typeof eventName !== 'string') {
190+
throw new TypeError('Event name must be a string.');
193191
}
194192

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

199-
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
200-
const list = this._getEventList(eventName, true);
201-
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
202-
// Already added.
203-
continue;
204-
}
205-
206-
list.push({
207-
callback,
208-
thisArg,
209-
once,
210-
});
197+
const list = this._getEventList(eventName, true);
198+
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
F438 199+
// Already added.
200+
return;
211201
}
202+
203+
list.push({
204+
callback,
205+
thisArg,
206+
once,
207+
});
212208
}
213209

214210
/**
215211
* Removes listener(s) for the specified event name.
216-
* @param eventNames Comma delimited names of the events the specified listener is associated with.
212+
* @param eventName Name of the event to attach to.
217213
* @param callback An optional parameter pointing to a specific listener. If not defined, all listeners for the event names will be removed.
218214
* @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.
219215
*/
220-
public removeEventListener(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
216+
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
221217
thisArg = thisArg || undefined;
222218

223-
if (typeof eventNames !== 'string') {
219+
if (typeof eventName !== 'string') {
224220
throw new TypeError('Events name(s) must be string.');
225221
}
226222

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

231-
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
232-
const entries = this._observers[eventName];
233-
if (!entries) {
234-
continue;
235-
}
227+
const entries = this._observers[eventName];
228+
if (!entries) {
229+
return;
230+
}
236231

237-
Observable.innerRemoveEventListener(entries, callback, thisArg);
232+
Observable.innerRemoveEventListener(entries, callback, thisArg);
238233

239-
if (!entries.length) {
240-
// Clear all entries of this type
241-
delete this._observers[eventName];
242-
}
234+
if (!entries.length) {
235+
// Clear all entries of this type
236+
delete this._observers[eventName];
243237
}
244238
}
245239

@@ -297,11 +291,11 @@ export class Observable {
297291
* in future.
298292
* @deprecated
299293
*/
300-
public static removeEventListener(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
294+
public static removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
301295
thisArg = thisArg || undefined;
302296

303-
if (typeof eventNames !== 'string') {
304-
throw new TypeError('Event name(s) must be a string.');
297+
if (typeof eventName !== 'string') {
298+
throw new TypeError('Event name must be a string.');
305299
}
306300

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

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

313-
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
314-
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
315-
if (!entries) {
316-
continue;
317-
}
307+
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
308+
if (!entries) {
309+
return;
310+
}
318311

319-
Observable.innerRemoveEventListener(entries, callback, thisArg);
312+
Observable.innerRemoveEventListener(entries, callback, thisArg);
320313

321-
if (!entries.length) {
322-
// Clear all entries of this type
323-
delete _globalEventHandlers[eventClass][eventName];
324-
}
314+
if (!entries.length) {
315+
// Clear all entries of this type
316+
delete _globalEventHandlers[eventClass][eventName];
317+
}
325318

326-
// Clear the primary class grouping if no list are left
327-
const keys = Object.keys(_globalEventHandlers[eventClass]);
328-
if (keys.length === 0) {
329-
delete _globalEventHandlers[eventClass];
330-
}
319+
// Clear the primary class grouping if no list are left
320+
const keys = Object.keys(_globalEventHandlers[eventClass]);
321+
if (keys.length === 0) {
322+
delete _globalEventHandlers[eventClass];
331323
}
332324
}
333325

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

343-
if (typeof eventNames !== 'string') {
344-
throw new TypeError('Event name(s) must be a string.');
335+
if (typeof eventName !== 'string') {
336+
throw new TypeError('Event name must be a string.');
345337
}
346338

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

356-
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
357-
if (!_globalEventHandlers[eventClass][eventName]) {
358-
_globalEventHandlers[eventClass][eventName] = [];
359-
}
360-
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
361-
// Already added.
362-
return;
363-
}
364-
365-
_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
348+
if (!_globalEventHandlers[eventClass][eventName]) {
349+
_globalEventHandlers[eventClass][eventName] = [];
350+
}
351+
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
352+
// Already added.
353+
return;
366354
}
355+
356+
_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
367357
}
368358

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

467-
public _emit(eventNames: string): void {
468-
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
469-
this.notify({ eventName, object: this });
470-
}
457+
public _emit(eventName: string): void {
458+
this.notify({ eventName, object: this });
471459
}
472460

473461
private _getEventList(eventName: string, createIfNeeded?: boolean): Array<ListenerEntry> | undefined {

‎packages/core/ui/core/bindable/index.ts

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { ViewBase } from '../view-base';
44
// Requires
55
import { unsetValue } from '../properties';
66
import { Observable, PropertyChangeData } from '../../../data/observable';
7+
import { fromString as gestureFromString } from '../../../ui/gestures/gestures-common';
78
import { addWeakEventListener, removeWeakEventListener } from '../weak-event-listener';
89
import { bindingConstants, parentsRegex } from '../../builder/binding-builder';
910
import { escapeRegexSymbols } from '../../../utils';
@@ -87,25 +88,45 @@ export interface ValueConverter {
8788
toView: (...params: any[]) => any;
8889
}
8990

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

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

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

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

108-
return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName.toLowerCase());
129+
return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName);
109130
}
110131

111132
return false;

‎packages/core/ui/core/view/view-common.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -305,11 +305,11 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {
305305

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

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

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

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

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

0 commit comments

Comments
 (0)
0