From 6dbff03b9bf5c94009acabe4bb0341e81223a8a6 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 18 Apr 2019 17:28:25 +0000 Subject: [PATCH] zjquery: Improve on/off handling for events. We no longer store handlers as an array of functions, and instead we assume that code will only ever set up one handler per sel/event or sel/event/child. This is almost always a sane policy for the app itself. We also try to improve error handling when devs write incorrect tests. The only tests that required changes here are the activity tests, which were a little careless about how data got reset between tests. --- frontend_tests/node_tests/activity.js | 18 +++- frontend_tests/zjsunit/zjquery.js | 120 +++++++++++++------------- 2 files changed, 77 insertions(+), 61 deletions(-) diff --git a/frontend_tests/node_tests/activity.js b/frontend_tests/node_tests/activity.js index 6cd0a49b6d..ab25341e42 100644 --- a/frontend_tests/node_tests/activity.js +++ b/frontend_tests/node_tests/activity.js @@ -324,7 +324,7 @@ function clear_buddy_list() { } function reset_setup() { - set_global('$', global.make_zjquery()); + $.clear_all_elements(); activity.set_cursor_and_filter(); buddy_list.container = $('#user_presences'); @@ -826,6 +826,15 @@ run_test('update_presence_info', () => { }); run_test('initialize', () => { + function clear() { + $.clear_all_elements(); + buddy_list.container = $('#user_presences'); + buddy_list.container.append = () => {}; + clear_buddy_list(); + } + + clear(); + $.stub_selector('html', { on: function (name, func) { func(); @@ -847,7 +856,10 @@ run_test('initialize', () => { }; activity.has_focus = false; + activity.initialize(); + clear(); + assert(scroll_handler_started); assert(!activity.new_user_input); assert(!$('#zephyr-mirror-error').hasClass('show')); @@ -864,16 +876,20 @@ run_test('initialize', () => { global.setInterval = (func) => func(); activity.initialize(); + assert($('#zephyr-mirror-error').hasClass('show')); assert(!activity.new_user_input); assert(!activity.has_focus); + clear(); + // Now execute the reload-in-progress code path _reload_state.is_in_progress = function () { return true; }; activity.initialize(); + clear(); }); run_test('away_status', () => { diff --git a/frontend_tests/zjsunit/zjquery.js b/frontend_tests/zjsunit/zjquery.js index ab3663ee02..d2b61594ba 100644 --- a/frontend_tests/zjsunit/zjquery.js +++ b/frontend_tests/zjsunit/zjquery.js @@ -34,91 +34,94 @@ exports.make_event_store = (selector) => { generic_event: generic_event, get_on_handler: function (name, child_selector) { - var funcs = self.get_on_handlers(name, child_selector); - var handler_name = name + ' with ' + child_selector; - if (funcs.length === 0) { - throw Error('Could not find handler for ' + handler_name); - } - if (funcs.length > 1) { - var remedy = "\nMaybe clear zjquery between tests?"; - throw Error('Found too many handlers for ' + handler_name + remedy); - } - return funcs[0]; - }, + var handler; - get_on_handlers: function (name, child_selector) { if (child_selector === undefined) { - return on_functions.get(name) || []; + handler = on_functions.get(name); + if (!handler) { + throw Error('no ' + name + ' handler for ' + selector); + } + return handler; } - var child_on = child_on_functions.get(child_selector) || {}; - if (!child_on) { - return []; + var child_on = child_on_functions.get(child_selector); + if (child_on) { + handler = child_on.get(name); } - return child_on.get(name) || []; + if (!handler) { + throw Error('no ' + name + ' handler for ' + selector + ' ' + child_selector); + } + + return handler; }, off: function () { - var event_name; + var event_name = arguments[0]; - if (arguments.length === 2) { - event_name = arguments[0]; - on_functions[event_name] = []; - } else if (arguments.length === 3) { - event_name = arguments[0]; - var sel = arguments[1]; - var child_on = child_on_functions.setdefault(sel, new Dict()); - child_on[event_name] = []; + if (arguments.length === 1) { + on_functions.del(event_name); + return; } + + // In the Zulip codebase we never use this form of + // .off in code that we test: $(...).off('click', child_sel); + // + // So we don't support this for now. + throw Error('zjquery does not support this call sequence'); }, on: function () { // parameters will either be // (event_name, handler) or // (event_name, sel, handler) - var event_name; + var event_name = arguments[0]; var sel; var handler; - // For each event_name (or event_name/sel combo), we will store an - // array of functions that are mapped to the event (or event/selector). - // - // Usually funcs is an array of just one element, but not always. - var funcs; - if (arguments.length === 2) { - event_name = arguments[0]; handler = arguments[1]; - funcs = on_functions.setdefault(event_name, []); - funcs.push(handler); - } else if (arguments.length === 3) { - event_name = arguments[0]; - sel = arguments[1]; - handler = arguments[2]; - assert.equal(typeof sel, 'string', 'String selectors expected here.'); - assert.equal(typeof handler, 'function', 'An handler function expected here.'); - var child_on = child_on_functions.setdefault(sel, new Dict()); - funcs = child_on.setdefault(event_name, []); - funcs.push(handler); + if (on_functions.has(event_name)) { + console.info('\nEither the app or the test can be at fault here..'); + console.info('(sometimes you just want to call $.clear_all_elements();)\n'); + throw Error('dup ' + event_name + ' handler for ' + selector); + } + + on_functions.set(event_name, handler); + return; } + + if (arguments.length !== 3) { + throw Error('wrong number of arguments passed in'); + } + + sel = arguments[1]; + handler = arguments[2]; + assert.equal(typeof sel, 'string', 'String selectors expected here.'); + assert.equal(typeof handler, 'function', 'An handler function expected here.'); + var child_on = child_on_functions.setdefault(sel, new Dict()); + + if (child_on.has(event_name)) { + throw Error('dup ' + event_name + ' handler for ' + selector + ' ' + sel); + } + + child_on.set(event_name, handler); }, trigger: function (ev) { var ev_name = typeof ev === 'string' ? ev : ev.name; - var funcs = on_functions.get(ev_name) || []; - // The following assertion is temporary. It can be - // legitimate for code to trigger multiple handlers. - // But up until now, we haven't needed this, and if - // you come across this assertion, it's possible that - // you can simplify your tests by just doing your own - // mocking of trigger(). If you really know what you - // are doing, you can remove this limitation. - assert(funcs.length <= 1, 'multiple functions set up'); + var func = on_functions.get(ev_name); - _.each(funcs, function (f) { - f(ev.data); - }); + if (!func) { + // It's possible that test code will trigger events + // that haven't been set up yet, but we are trying to + // eventually deprecate trigger in our codebase, so for + // now we just let calls to trigger silently do nothing. + // (And I think actual jQuery would do the same thing.) + return; + } + + func(ev.data); }, }; @@ -220,9 +223,6 @@ exports.make_new_elem = function (selector, opts) { get_on_handler: function (name, child_selector) { return event_store.get_on_handler(name, child_selector); }, - get_on_handlers: function (name, child_selector) { - return event_store.get_on_handlers(name, child_selector); - }, hasClass: function (class_name) { return classes.has(class_name); },