From ee3e488e02cb027f1546dbae6420e418ce21cba0 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 26 Dec 2019 14:34:17 +0000 Subject: [PATCH] js: Extract FoldDict class. We have ~5 years of proof that we'll probably never extend Dict with more options. Breaking the classes into makes both a little faster (no options to check), and we remove some options in FoldDict that are never used (from/from_array). A possible next step is to fine-tune the Dict to use Map internally. Note that the TypeScript types for FoldDict are now more specific (requiring string keys). Of course, this isn't really enforced until we convert other modules to TS. --- frontend_tests/node_tests/dict.js | 33 +------- frontend_tests/node_tests/fold_dict.js | 98 ++++++++++++++++++++++++ frontend_tests/node_tests/stream_list.js | 3 +- frontend_tests/node_tests/unread.js | 3 +- static/js/bundles/app.js | 1 + static/js/dict.ts | 36 ++------- static/js/fold_dict.ts | 85 ++++++++++++++++++++ static/js/muting.js | 3 +- static/js/people.js | 7 +- static/js/pm_conversations.js | 3 +- static/js/recent_senders.js | 3 +- static/js/stream_data.js | 8 +- static/js/topic_data.js | 3 +- static/js/topic_list.js | 3 +- static/js/unread.js | 6 +- static/js/user_groups.js | 3 +- tools/test-js-with-node | 1 + 17 files changed, 223 insertions(+), 76 deletions(-) create mode 100644 frontend_tests/node_tests/fold_dict.js create mode 100644 static/js/fold_dict.ts diff --git a/frontend_tests/node_tests/dict.js b/frontend_tests/node_tests/dict.js index 9be564b380..89833838f5 100644 --- a/frontend_tests/node_tests/dict.js +++ b/frontend_tests/node_tests/dict.js @@ -13,10 +13,12 @@ run_test('basic', () => { d.set('foo', 'baz'); assert.equal(d.get('foo'), 'baz'); + assert.equal(d.num_items(), 1); d.set('bar', 'qux'); assert.equal(d.get('foo'), 'baz'); assert.equal(d.get('bar'), 'qux'); + assert.equal(d.num_items(), 2); assert.equal(d.has('bar'), true); assert.equal(d.has('baz'), false); @@ -52,41 +54,14 @@ run_test('filter_values', () => { assert.deepEqual(d.filter_values(pred).sort(), ['fay', 'foo', 'fred']); }); -run_test('fold_case', () => { - const d = new Dict({fold_case: true}); - - assert.deepEqual(d.keys(), []); - - assert(!d.has('foo')); - d.set('fOO', 'Hello World'); - assert.equal(d.get('foo'), 'Hello World'); - assert(d.has('foo')); - assert(d.has('FOO')); - assert(!d.has('not_a_key')); - - assert.deepEqual(d.keys(), ['fOO']); - - d.del('Foo'); - assert.equal(d.has('foo'), false); - - assert.deepEqual(d.keys(), []); -}); - run_test('undefined_keys', () => { blueslip.set_test_data('error', 'Tried to call a Dict method with an undefined key.'); - let d = new Dict(); + const d = new Dict(); assert.equal(d.has(undefined), false); assert.strictEqual(d.get(undefined), undefined); - - d = new Dict({fold_case: true}); - - assert.equal(d.has(undefined), false); - assert.strictEqual(d.get(undefined), undefined); - assert.equal(blueslip.get_test_logs('error').length, 4); - - blueslip.clear_test_data(); + assert.equal(blueslip.get_test_logs('error').length, 2); }); run_test('restricted_keys', () => { diff --git a/frontend_tests/node_tests/fold_dict.js b/frontend_tests/node_tests/fold_dict.js new file mode 100644 index 0000000000..7f27176b90 --- /dev/null +++ b/frontend_tests/node_tests/fold_dict.js @@ -0,0 +1,98 @@ +const FoldDict = zrequire('fold_dict').FoldDict; +set_global('blueslip', global.make_zblueslip()); + +run_test('basic', () => { + const d = new FoldDict(); + + assert(d.is_empty()); + + assert.deepEqual(d.keys(), []); + + d.set('foo', 'bar'); + assert.equal(d.get('foo'), 'bar'); + assert(!d.is_empty()); + + d.set('foo', 'baz'); + assert.equal(d.get('foo'), 'baz'); + assert.equal(d.num_items(), 1); + + d.set('bar', 'qux'); + assert.equal(d.get('foo'), 'baz'); + assert.equal(d.get('bar'), 'qux'); + assert.equal(d.num_items(), 2); + + assert.equal(d.has('bar'), true); + assert.equal(d.has('baz'), false); + + assert.deepEqual(d.keys(), ['foo', 'bar']); + assert.deepEqual(d.values(), ['baz', 'qux']); + assert.deepEqual(d.items(), [['foo', 'baz'], ['bar', 'qux']]); + + d.del('bar'); + assert.equal(d.has('bar'), false); + assert.strictEqual(d.get('bar'), undefined); + + assert.deepEqual(d.keys(), ['foo']); + + const val = ['foo']; + const res = d.set('abc', val); + assert.equal(val, res); +}); + +run_test('case insensitivity', () => { + const d = new FoldDict(); + + assert.deepEqual(d.keys(), []); + + assert(!d.has('foo')); + d.set('fOO', 'Hello World'); + assert.equal(d.get('foo'), 'Hello World'); + assert(d.has('foo')); + assert(d.has('FOO')); + assert(!d.has('not_a_key')); + + assert.deepEqual(d.keys(), ['fOO']); + + d.del('Foo'); + assert.equal(d.has('foo'), false); + + assert.deepEqual(d.keys(), []); +}); + +run_test('clear', () => { + const d = new FoldDict(); + + function populate() { + d.set('fOO', 1); + assert.equal(d.get('foo'), 1); + d.set('bAR', 2); + assert.equal(d.get('bar'), 2); + } + + populate(); + assert.equal(d.num_items(), 2); + assert(!d.is_empty()); + + d.clear(); + assert.equal(d.get('fOO'), undefined); + assert.equal(d.get('bAR'), undefined); + assert.equal(d.num_items(), 0); + assert(d.is_empty()); + + // make sure it still works after clearing + populate(); + assert.equal(d.num_items(), 2); +}); + +run_test('undefined_keys', () => { + blueslip.set_test_data('error', 'Tried to call a FoldDict method with an undefined key.'); + + const d = new FoldDict(); + + assert.equal(d.has(undefined), false); + assert.strictEqual(d.get(undefined), undefined); + assert.equal(blueslip.get_test_logs('error').length, 2); + + blueslip.clear_test_data(); +}); + diff --git a/frontend_tests/node_tests/stream_list.js b/frontend_tests/node_tests/stream_list.js index b2bc73353f..276c6b48b4 100644 --- a/frontend_tests/node_tests/stream_list.js +++ b/frontend_tests/node_tests/stream_list.js @@ -1,6 +1,7 @@ set_global('document', 'document-stub'); set_global('$', global.make_zjquery()); +const FoldDict = zrequire('fold_dict').FoldDict; zrequire('unread_ui'); zrequire('Filter', 'js/filter'); zrequire('util'); @@ -670,7 +671,7 @@ run_test('update_count_in_dom', () => { }; }; - const topic_count = new Dict({fold_case: true}); + const topic_count = new FoldDict(); topic_count.set('lunch', '555'); counts.topic_count.set(stream_id, topic_count); diff --git a/frontend_tests/node_tests/unread.js b/frontend_tests/node_tests/unread.js index 8492d890c8..7c62f74b28 100644 --- a/frontend_tests/node_tests/unread.js +++ b/frontend_tests/node_tests/unread.js @@ -6,6 +6,7 @@ zrequire('stream_data'); zrequire('util'); zrequire('unread'); zrequire('settings_notifications'); +const FoldDict = zrequire('fold_dict').FoldDict; set_global('page_params', {}); set_global('blueslip', {}); @@ -285,7 +286,7 @@ run_test('num_unread_for_topic', () => { msg_ids = unread.get_msg_ids_for_stream(stream_id); assert.deepEqual(msg_ids, _.range(1, 501)); - const topic_dict = new Dict({fold_case: true}); + const topic_dict = new FoldDict(); let missing_topics = unread.get_missing_topics({ stream_id: stream_id, diff --git a/static/js/bundles/app.js b/static/js/bundles/app.js index 9998464493..b19f82e48f 100644 --- a/static/js/bundles/app.js +++ b/static/js/bundles/app.js @@ -33,6 +33,7 @@ import "../lightbox_canvas.js"; import "../rtl.js"; import "../lazy_set.js"; import "../dict.ts"; +import "../fold_dict.ts"; import "../scroll_util.js"; import "../components.js"; import "../feedback_widget.js"; diff --git a/static/js/dict.ts b/static/js/dict.ts index 48e3cb634c..b5f38f9fb4 100644 --- a/static/js/dict.ts +++ b/static/js/dict.ts @@ -1,44 +1,23 @@ import * as _ from 'underscore'; -/** - * Implementation detail of the Dict class. `key` is `k` converted to a string, - * in lowercase if the `fold_case` option is enabled. - */ type KeyValue = { k: K; v: V }; type Items = { [key: string]: KeyValue; }; -/** - * This class primarily exists to support the fold_case option, because so many - * string keys in Zulip are case-insensitive (emails, stream names, topics, - * etc.). Dict also accepts any key that can be converted to a string. - */ export class Dict { private _items: Items = {}; - private _fold_case: boolean; - - /** - * @param opts - setting `fold_case` to true will make `has()` and `get()` - * case-insensitive. `keys()` and other methods that - * implicitly return keys return the original casing/type - * of the key passed into `set()`. - */ - constructor(opts?: {fold_case: boolean}) { - this._fold_case = opts ? opts.fold_case : false; - } /** * Constructs a Dict object from an existing object's keys and values. * @param obj - A javascript object - * @param opts - Options to be passed to the Dict constructor */ - static from(obj: { [key: string]: V }, opts?: {fold_case: boolean}): Dict { + static from(obj: { [key: string]: V }): Dict { if (typeof obj !== "object" || obj === null) { throw new TypeError("Cannot convert argument to Dict"); } - const dict = new Dict(opts); + const dict = new Dict(); _.each(obj, function (val: V, key: string) { dict.set(key, val); }); @@ -50,14 +29,13 @@ export class Dict { * Construct a Dict object from an array with each element set to `true`. * Intended for use as a set data structure. * @param arr - An array of keys - * @param opts - Options to be passed to the Dict constructor */ - static from_array(arr: K[], opts?: {fold_case: boolean}): Dict { + static from_array(arr: K[]): Dict { if (!(arr instanceof Array)) { throw new TypeError("Argument is not an array"); } - const dict = new Dict(opts); + const dict = new Dict(); for (const key of arr) { dict.set(key, true); } @@ -65,7 +43,7 @@ export class Dict { } clone(): Dict { - const dict = new Dict({fold_case: this._fold_case}); + const dict = new Dict(); dict._items = { ...this._items }; return dict; } @@ -144,13 +122,13 @@ export class Dict { this._items = {}; } - // Handle case-folding of keys and the empty string. + // Convert keys to strings and handle undefined. private _munge(key: K): string | undefined { if (key === undefined) { blueslip.error("Tried to call a Dict method with an undefined key."); return undefined; } const str_key = ':' + key.toString(); - return this._fold_case ? str_key.toLowerCase() : str_key; + return str_key; } } diff --git a/static/js/fold_dict.ts b/static/js/fold_dict.ts new file mode 100644 index 0000000000..cc0ad7c43d --- /dev/null +++ b/static/js/fold_dict.ts @@ -0,0 +1,85 @@ +import * as _ from 'underscore'; + +/* + Use this class to manage keys where you don't care + about case (i.e. case-insensitive). + + Keys for FoldDict should be strings. We "fold" all + casings of "alice" (e.g. "ALICE", "Alice", "ALIce", etc.) + to "alice" as the key. + + Examples of case-insensitive data in Zulip are: + - emails + - stream names + - topics + - etc. + */ +type KeyValue = { k: string; v: V }; +type Items = { + [key: string]: KeyValue; +}; + +export class FoldDict { + private _items: Items = {}; + + get(key: string): V | undefined { + const mapping = this._items[this._munge(key)]; + if (mapping === undefined) { + return undefined; + } + return mapping.v; + } + + set(key: string, value: V): V { + this._items[this._munge(key)] = {k: key, v: value}; + return value; + } + + has(key: string): boolean { + return _.has(this._items, this._munge(key)); + } + + del(key: string): void { + delete this._items[this._munge(key)]; + } + + keys(): string[] { + return _.pluck(_.values(this._items), 'k'); + } + + values(): V[] { + return _.pluck(_.values(this._items), 'v'); + } + + items(): [string, V][] { + return _.map(_.values(this._items), + (mapping: KeyValue): [string, V] => [mapping.k, mapping.v]); + } + + num_items(): number { + return _.keys(this._items).length; + } + + is_empty(): boolean { + return _.isEmpty(this._items); + } + + each(f: (v: V, k?: string) => void): void { + _.each(this._items, (mapping: KeyValue) => f(mapping.v, mapping.k)); + } + + clear(): void { + this._items = {}; + } + + // Handle case-folding of keys and the empty string. + private _munge(key: string): string | undefined { + if (key === undefined) { + blueslip.error("Tried to call a FoldDict method with an undefined key."); + return undefined; + } + + const str_key = ':' + key.toString().toLowerCase(); + return str_key; + } +} diff --git a/static/js/muting.js b/static/js/muting.js index d089b119d6..0176b51fe5 100644 --- a/static/js/muting.js +++ b/static/js/muting.js @@ -1,11 +1,12 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; let muted_topics = new Dict(); exports.add_muted_topic = function (stream_id, topic) { let sub_dict = muted_topics.get(stream_id); if (!sub_dict) { - sub_dict = new Dict({fold_case: true}); + sub_dict = new FoldDict(); muted_topics.set(stream_id, sub_dict); } sub_dict.set(topic, true); diff --git a/static/js/people.js b/static/js/people.js index 2df152d561..8cf04368f6 100644 --- a/static/js/people.js +++ b/static/js/people.js @@ -1,5 +1,6 @@ require("unorm"); // String.prototype.normalize polyfill for IE11 const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; let people_dict; let people_by_name_dict; @@ -17,8 +18,8 @@ exports.init = function () { // (all people we've seen), but people_dict can have duplicate // keys related to email changes. We want to deprecate // people_dict over time and always do lookups by user_id. - people_dict = new Dict({fold_case: true}); - people_by_name_dict = new Dict({fold_case: true}); + people_dict = new FoldDict(); + people_by_name_dict = new FoldDict(); people_by_user_id_dict = new Dict(); // The next dictionary includes all active users (human/user) @@ -29,7 +30,7 @@ exports.init = function () { pm_recipient_count_dict = new Dict(); // The next Dict maintains a set of ids of people with same full names. - duplicate_full_name_data = new Dict({fold_case: true}); + duplicate_full_name_data = new FoldDict(); }; // WE INITIALIZE DATA STRUCTURES HERE! diff --git a/static/js/pm_conversations.js b/static/js/pm_conversations.js index c044004808..bf8f1224f6 100644 --- a/static/js/pm_conversations.js +++ b/static/js/pm_conversations.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; const partners = new Dict(); @@ -15,7 +16,7 @@ exports.recent = (function () { // recent conversations with, sorted by time (implemented via // `message_id` sorting, since that's how we time-sort messages). const self = {}; - const recent_message_ids = new Dict({fold_case: true}); // key is user_ids_string + const recent_message_ids = new FoldDict(); // key is user_ids_string const recent_private_messages = []; self.insert = function (user_ids, message_id) { diff --git a/static/js/recent_senders.js b/static/js/recent_senders.js index 5917fb3fd7..c3e98bd4e3 100644 --- a/static/js/recent_senders.js +++ b/static/js/recent_senders.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; const topic_senders = new Dict(); // key is stream-id, value is Dict const stream_senders = new Dict(); // key is stream-id, value is Dict @@ -8,7 +9,7 @@ exports.process_message_for_senders = function (message) { const topic = util.get_message_topic(message); // Process most recent sender to topic - const topic_dict = topic_senders.get(stream_id) || new Dict({fold_case: true}); + const topic_dict = topic_senders.get(stream_id) || new FoldDict(); let sender_message_ids = topic_dict.get(topic) || new Dict(); let old_message_id = sender_message_ids.get(message.sender_id); diff --git a/static/js/stream_data.js b/static/js/stream_data.js index d5ec758a13..09884c5968 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; const LazySet = require('./lazy_set').LazySet; const BinaryDict = function (pred) { @@ -18,8 +19,8 @@ const BinaryDict = function (pred) { */ const self = {}; - self.trues = new Dict({fold_case: true}); - self.falses = new Dict({fold_case: true}); + self.trues = new FoldDict(); + self.falses = new FoldDict(); self.true_values = function () { return self.trues.values(); @@ -83,13 +84,12 @@ let stream_info; let subs_by_stream_id; let filter_out_inactives = false; -const stream_ids_by_name = new Dict({fold_case: true}); +const stream_ids_by_name = new FoldDict(); exports.clear_subscriptions = function () { stream_info = new BinaryDict(function (sub) { return sub.subscribed; }); - subs_by_stream_id = new Dict(); }; diff --git a/static/js/topic_data.js b/static/js/topic_data.js index 29c784d2c1..65cd3836ce 100644 --- a/static/js/topic_data.js +++ b/static/js/topic_data.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; let stream_dict = new Dict(); // stream_id -> array of objects @@ -13,7 +14,7 @@ exports.stream_has_topics = function (stream_id) { }; exports.topic_history = function (stream_id) { - const topics = new Dict({fold_case: true}); + const topics = new FoldDict(); const self = {}; diff --git a/static/js/topic_list.js b/static/js/topic_list.js index 9ec5af6a08..d8032e2ee7 100644 --- a/static/js/topic_list.js +++ b/static/js/topic_list.js @@ -1,6 +1,7 @@ const render_more_topics = require('../templates/more_topics.hbs'); const render_topic_list_item = require('../templates/topic_list_item.hbs'); const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; /* Track all active widgets with a Dict. @@ -84,7 +85,7 @@ exports.widget = function (parent_elem, my_stream_id) { const self = {}; self.build_list = function () { - self.topic_items = new Dict({fold_case: true}); + self.topic_items = new FoldDict(); let topics_selected = 0; let more_topics_unreads = 0; diff --git a/static/js/unread.js b/static/js/unread.js index 9db0858f98..8373f5be97 100644 --- a/static/js/unread.js +++ b/static/js/unread.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; // See https://zulip.readthedocs.io/en/latest/subsystems/pointer.html for notes on // how this system is designed. @@ -65,8 +66,7 @@ const unread_messages = make_id_set(); function make_bucketer(options) { const self = {}; - - const key_to_bucket = new Dict({fold_case: options.fold_case}); + const key_to_bucket = options.fold_case ? new FoldDict() : new Dict(); const reverse_lookup = new Dict(); self.clear = function () { @@ -273,7 +273,7 @@ exports.unread_topic_counter = (function () { function str_dict() { // Use this when keys are topics - return new Dict({fold_case: true}); + return new FoldDict(); } function num_dict() { diff --git a/static/js/user_groups.js b/static/js/user_groups.js index a3b2f7c252..eb6023054d 100644 --- a/static/js/user_groups.js +++ b/static/js/user_groups.js @@ -1,4 +1,5 @@ const Dict = require('./dict').Dict; +const FoldDict = require('./fold_dict').FoldDict; let user_group_name_dict; let user_group_by_id_dict; @@ -6,7 +7,7 @@ let user_group_by_id_dict; // We have an init() function so that our automated tests // can easily clear data. exports.init = function () { - user_group_name_dict = new Dict({fold_case: true}); + user_group_name_dict = new FoldDict(); user_group_by_id_dict = new Dict(); }; diff --git a/tools/test-js-with-node b/tools/test-js-with-node index 398cfeec3c..34cf9847c4 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -47,6 +47,7 @@ enforce_fully_covered = { 'static/js/fenced_code.js', 'static/js/fetch_status.js', 'static/js/filter.js', + 'static/js/fold_dict.ts', 'static/js/hash_util.js', 'static/js/keydown_util.js', 'static/js/input_pill.js',