ui_init: Handle page_params more cleanly.

This cleans up the handoff of page_params
data between ui_init and modules that
take over ownership of page_params-derived
data.

Read the long comment in ui_init for a bit
more context.

Most of this diff is actually test cleanup.
And a lot of the diff to "real" code is
just glorified `s/page_params/params/`
in the `initialize` functions.

One little oddity is that we don't actually
surrender ownership of `page_params.user_id`
to `people.js`.  We could plausibly sweep
the rest of the codebase to just use
`people.my_user_id()` consistently, but it's
not a super high priority thing to fix,
since the value never changes.

The stream_data situation is a bit messy,
since we consume `page_params` data in the
initialize() function in addition to the
`params` data we "own".  I added a comment
there and intend to follow up.  I tried
to mostly avoid the "word soup" by extracting
three locals at the top.

Finally, I don't touch `alert_words` yet,
despite it also doing the delete-page-params-data
dance.  The problem is that `alert_words`
doesn't have a proper `initialize()`.  We
should clean that up and have it use a
`Map` internally, too.

frontend_tests/node_tests/bot_data.js

@@ -2,15 +2,14 @@ const _settings_bots = {
     render_bots: () => {},
 };
 
-const _page_params = {
+const bot_data_params = {
     realm_bots: [{email: 'bot0@zulip.com', user_id: 42, full_name: 'Bot 0',
                   services: []},
                  {email: 'outgoingwebhook@zulip.com', user_id: 314, full_name: "Outgoing webhook",
                   services: [{base_url: "http://foo.com", interface: 1}]}],
-    is_admin: false,
 };
 
-set_global('page_params', _page_params);
+set_global('page_params', {is_admin: false});
 set_global('settings_bots', _settings_bots);
 
 zrequire('people');
@@ -24,7 +23,7 @@ global.people.add({
 
 global.people.initialize_current_user(42);
 
-bot_data.initialize();
+bot_data.initialize(bot_data_params);
 // Our startup logic should have added Bot 0 from page_params.
 assert.equal(bot_data.get(42).full_name, 'Bot 0');
 assert.equal(bot_data.get(314).full_name, 'Outgoing webhook');

frontend_tests/node_tests/compose.js

@@ -276,11 +276,16 @@ run_test('get_invalid_recipient_emails', () => {
         user_id: 124,
         full_name: 'Welcome Bot',
     };
-    page_params.realm_users = [];
+
-    page_params.realm_non_active_users = [];
-    page_params.cross_realm_bots = [welcome_bot];
     page_params.user_id = 30;
-    people.initialize();
+
+    const params = {};
+    params.realm_users = [];
+    params.realm_non_active_users = [];
+    params.cross_realm_bots = [welcome_bot];
+
+    people.initialize(page_params.user_id, params);
+
     compose_state.private_message_recipient('welcome-bot@example.com');
     assert.deepEqual(compose.get_invalid_recipient_emails(), []);
 });

frontend_tests/node_tests/echo.js

@@ -91,7 +91,9 @@ run_test('process_from_server for differently rendered messages', () => {
 
 run_test('build_display_recipient', () => {
     page_params.user_id = 123;
-    page_params.realm_users = [
+
+    const params = {};
+    params.realm_users = [
         {
             user_id: 123,
             full_name: "Iago",
@@ -103,9 +105,9 @@ run_test('build_display_recipient', () => {
             user_id: 21,
         },
     ];
-    page_params.realm_non_active_users = [];
+    params.realm_non_active_users = [];
-    page_params.cross_realm_bots = [];
+    params.cross_realm_bots = [];
-    people.initialize();
+    people.initialize(page_params.user_id, params);
 
     let message = {
         type: "stream",
@@ -160,16 +162,18 @@ run_test('insert_local_message', () => {
     const local_id_float = 1;
 
     page_params.user_id = 123;
-    page_params.realm_users = [
+
+    const params = {};
+    params.realm_users = [
         {
             user_id: 123,
             full_name: "Iago",
             email: "iago@zulip.com",
         },
     ];
-    page_params.realm_non_active_users = [];
+    params.realm_non_active_users = [];
-    page_params.cross_realm_bots = [];
+    params.cross_realm_bots = [];
-    people.initialize();
+    people.initialize(page_params.user_id, params);
 
     let apply_markdown_called = false;
     let add_topic_links_called = false;

frontend_tests/node_tests/people.js

@@ -877,7 +877,9 @@ run_test('track_duplicate_full_names', () => {
 run_test('initialize', () => {
     people.init();
 
-    global.page_params.realm_non_active_users = [
+    const params = {};
+
+    params.realm_non_active_users = [
         {
             email: 'retiree@example.com',
             user_id: 15,
@@ -885,23 +887,23 @@ run_test('initialize', () => {
         },
     ];
 
-    global.page_params.realm_users = [
+    params.realm_users = [
         {
             email: 'alice@example.com',
             user_id: 16,
             full_name: 'Alice',
         },
     ];
-    global.page_params.cross_realm_bots = [
+    params.cross_realm_bots = [
         {
             email: 'bot@example.com',
             user_id: 17,
             full_name: 'Test Bot',
         },
     ];
-    global.page_params.user_id = 42;
 
-    people.initialize();
+    const my_user_id = 42;
+    people.initialize(my_user_id, params);
 
     assert.equal(people.get_active_user_for_email('alice@example.com').full_name, 'Alice');
     assert.equal(people.is_active_user_for_popover(17), true);

frontend_tests/node_tests/presence.js

@@ -5,7 +5,6 @@ const return_false = function () { return false; };
 
 set_global('server_events', {});
 set_global('blueslip', {});
-set_global('page_params', {});
 set_global('reload_state', {
     is_in_progress: return_false,
 });
@@ -161,11 +160,10 @@ run_test('set_presence_info', () => {
         },
     };
 
-    page_params.presences = presences;
+    const params = {};
-    page_params.initial_servertime = base_time;
+    params.presences = presences;
-    presence.initialize();
+    params.initial_servertime = base_time;
-
+    presence.initialize(params);
-    assert.equal(page_params.presences, undefined);
 
     assert.deepEqual(presence.presence_info.get(alice.user_id),
                      { status: 'active', last_active: 500}

frontend_tests/node_tests/settings_bots.js

@@ -5,13 +5,16 @@ set_global("page_params", {
         {name: "giphy", config: {key: "12345678"}},
         {name: "foobot", config: {bar: "baz", qux: "quux"}},
     ],
+});
+
+const bot_data_params = {
     realm_bots: [{api_key: 'QadL788EkiottHmukyhHgePUFHREiu8b',
                   email: 'error-bot@zulip.org',
                   full_name: 'Error bot',
                   user_id: 1,
                   services: []},
     ],
-});
+};
 
 set_global("avatar", {});
 
@@ -26,7 +29,7 @@ set_global('ClipboardJS', function (sel) {
     assert.equal(sel, '#copy_zuliprc');
 });
 
-bot_data.initialize();
+bot_data.initialize(bot_data_params);
 
 run_test('generate_zuliprc_uri', () => {
     const uri = settings_bots.generate_zuliprc_uri(1);

frontend_tests/node_tests/stream_data.js

@@ -770,27 +770,35 @@ run_test('create_sub', () => {
 });
 
 run_test('initialize', () => {
-    function initialize() {
+    function get_params() {
-        page_params.subscriptions = [{
+        const params = {};
+
+        params.subscriptions = [{
             name: 'subscriptions',
             stream_id: 2001,
         }];
 
-        page_params.unsubscribed = [{
+        params.unsubscribed = [{
             name: 'unsubscribed',
             stream_id: 2002,
         }];
 
-        page_params.never_subscribed = [{
+        params.never_subscribed = [{
             name: 'never_subscribed',
             stream_id: 2003,
         }];
+
+        return params;
+    }
+
+    function initialize() {
+        stream_data.initialize(get_params());
     }
 
-    initialize();
     page_params.demote_inactive_streams = 1;
     page_params.realm_notifications_stream_id = -1;
-    stream_data.initialize();
+
+    initialize();
     assert(!stream_data.is_filtering_inactives());
 
     const stream_names = stream_data.get_streams_for_admin().map(elem => elem.name);
@@ -803,9 +811,8 @@ run_test('initialize', () => {
     assert.equal(page_params.notifications_stream, "");
 
     // Simulate a private stream the user isn't subscribed to
-    initialize();
     page_params.realm_notifications_stream_id = 89;
-    stream_data.initialize();
+    initialize();
     assert.equal(page_params.notifications_stream, "");
 
     // Now actually subscribe the user to the stream
@@ -816,16 +823,17 @@ run_test('initialize', () => {
     };
 
     stream_data.add_sub(foo);
-    stream_data.initialize();
+    initialize();
     assert.equal(page_params.notifications_stream, "foo");
 });
 
 run_test('filter inactives', () => {
-    page_params.unsubscribed = [];
+    const params = {};
-    page_params.never_subscribed = [];
+    params.unsubscribed = [];
-    page_params.subscriptions = [];
+    params.never_subscribed = [];
+    params.subscriptions = [];
 
-    stream_data.initialize();
+    stream_data.initialize(params);
     assert(!stream_data.is_filtering_inactives());
 
     page_params.unsubscribed = [];
@@ -844,7 +852,7 @@ run_test('filter inactives', () => {
         };
         stream_data.add_sub(sub);
     });
-    stream_data.initialize();
+    stream_data.initialize(params);
     assert(stream_data.is_filtering_inactives());
 });
 

frontend_tests/node_tests/user_groups.js

@@ -1,5 +1,4 @@
 set_global('blueslip', global.make_zblueslip());
-set_global('page_params', {});
 
 zrequire('user_groups');
 
@@ -9,9 +8,11 @@ run_test('user_groups', () => {
         id: 0,
         members: [1, 2],
     };
-    global.page_params.realm_user_groups = [students];
 
-    user_groups.initialize();
+    const params = {};
+    params.realm_user_groups = [students];
+
+    user_groups.initialize(params);
     assert.equal(user_groups.get_user_group_from_id(students.id), students);
 
     const admins = {

frontend_tests/node_tests/user_status.js

@@ -1,15 +1,16 @@
 set_global('blueslip', global.make_zblueslip());
 set_global('channel', {});
-set_global('page_params', {});
 zrequire('user_status');
 
 function initialize() {
-    page_params.user_status = {
+    const params = {
-        1: {away: true, status_text: 'in a meeting'},
+        user_status: {
-        2: {away: true},
+            1: {away: true, status_text: 'in a meeting'},
-        3: {away: true},
+            2: {away: true},
+            3: {away: true},
+        },
     };
-    user_status.initialize();
+    user_status.initialize(params);
 }
 
 run_test('basics', () => {
@@ -23,7 +24,6 @@ run_test('basics', () => {
     user_status.revoke_away(4);
     assert(!user_status.is_away(4));
 
-    // use value from page_params
     assert.equal(user_status.get_status_text(1), 'in a meeting');
 
     user_status.set_status_text({

static/js/bot_data.js

@@ -86,12 +86,10 @@ exports.get_services = function bot_data__get_services(bot_id) {
     return services.get(bot_id);
 };
 
-exports.initialize = function () {
+exports.initialize = function (params) {
-    for (const bot of page_params.realm_bots) {
+    for (const bot of params.realm_bots) {
         exports.add(bot);
     }
-
-    delete page_params.realm_bots;
 };
 
 window.bot_data = exports;

static/js/people.js

@@ -1188,27 +1188,23 @@ exports.is_my_user_id = function (user_id) {
     return user_id === my_user_id;
 };
 
-exports.initialize = function () {
+exports.initialize = function (my_user_id, params) {
-    for (const person of page_params.realm_users) {
+    for (const person of params.realm_users) {
         exports.add_in_realm(person);
     }
 
-    for (const person of page_params.realm_non_active_users) {
+    for (const person of params.realm_non_active_users) {
         exports.add(person);
     }
 
-    for (const person of page_params.cross_realm_bots) {
+    for (const person of params.cross_realm_bots) {
         if (!people_dict.has(person.email)) {
             exports.add(person);
         }
         cross_realm_dict.set(person.user_id, person);
     }
 
-    exports.initialize_current_user(page_params.user_id);
+    exports.initialize_current_user(my_user_id);
-
-    delete page_params.realm_users; // We are the only consumer of this.
-    delete page_params.realm_non_active_users;
-    delete page_params.cross_realm_bots;
 };
 
 window.people = exports;

static/js/presence.js

@@ -186,10 +186,9 @@ exports.last_active_date = function (user_id) {
     return date;
 };
 
-exports.initialize = function () {
+exports.initialize = function (params) {
-    presence.set_info(page_params.presences,
+    presence.set_info(params.presences,
-                      page_params.initial_servertime);
+                      params.initial_servertime);
-    delete page_params.presences;
 };
 
 window.presence = exports;

static/js/stream_data.js

@@ -788,8 +788,31 @@ exports.get_streams_for_admin = function () {
     return subs;
 };
 
-exports.initialize = function () {
+exports.initialize = function (params) {
-    color_data.claim_colors(page_params.subscriptions);
+    /*
+        We get `params` data, which is data that we "own"
+        and which has already been removed from `page_params`.
+        We only use it in this function to populate other
+        data structures.
+    */
+
+    const subscriptions = params.subscriptions;
+    const unsubscribed = params.unsubscribed;
+    const never_subscribed = params.never_subscribed;
+
+    /*
+        We also consume some data directly from `page_params`.
+        This data can be accessed by any other module,
+        and we consider the authoritative source to be
+        `page_params`.  Some of this data should eventually
+        be fully handled by stream_data.  In particular,
+        the way we handle `page_params.realm_default_streams`
+        is kinda janky, because we maintain our own data
+        structure, but then some legacy modules still
+        refer directly to `page_params`.  We should fix that.
+    */
+
+    color_data.claim_colors(subscriptions);
 
     function populate_subscriptions(subs, subscribed, previously_subscribed) {
         subs.forEach(function (sub) {
@@ -803,9 +826,9 @@ exports.initialize = function () {
 
     exports.set_realm_default_streams(page_params.realm_default_streams);
 
-    populate_subscriptions(page_params.subscriptions, true, true);
+    populate_subscriptions(subscriptions, true, true);
-    populate_subscriptions(page_params.unsubscribed, false, true);
+    populate_subscriptions(unsubscribed, false, true);
-    populate_subscriptions(page_params.never_subscribed, false, false);
+    populate_subscriptions(never_subscribed, false, false);
 
     // Migrate the notifications stream from the new API structure to
     // what the frontend expects.
@@ -824,11 +847,6 @@ exports.initialize = function () {
     }
 
     exports.set_filter_out_inactives();
-
-    // Garbage collect data structures that were only used for initialization.
-    delete page_params.subscriptions;
-    delete page_params.unsubscribed;
-    delete page_params.never_subscribed;
 };
 
 exports.remove_default_stream = function (stream_id) {

static/js/ui_init.js

@@ -295,16 +295,133 @@ exports.initialize_kitchen_sink_stuff = function () {
 };
 
 exports.initialize_everything = function () {
-    // initialize other stuff
+    /*
+        When we initialize our various modules, a lot
+        of them will consume data from the server
+        in the form of `page_params`.
+
+        The global `page_params` var is basically
+        a massive dictionary with all the information
+        that the client needs to run the app.  Here
+        are some examples of what it includes:
+
+            - all of the user's user-specific settings
+            - all realm-specific settings that are
+              pertinent to the user
+            - info about streams/subscribers on the realm
+            - realm settings
+            - info about all the other users
+            - some fairly dynamic data, like which of
+              the other users are "present"
+
+        Except for the actual Zulip messages, basically
+        any data that you see in the app soon after page
+        load comes from `page_params`.
+
+        ## Mostly static data
+
+        Now, we mostly leave `page_params` intact through
+        the duration of the app.  Most of the data in
+        `page_params` is fairly static in nature, and we
+        will simply update it for basic changes like
+        the following (meant as examples, not gospel):
+
+            - I changed my 24-hour time preference.
+            - The realm admin changed who can edit topics.
+            - The team's realm icon has changed.
+            - I switched from day mode to night mode.
+
+        Especially for things that are settings-related,
+        we rarely abstract away the data from `page_params`.
+        As of this writing, over 90 modules refer directly
+        to `page_params` for some reason or another.
+
+        ## Dynamic data
+
+        Some of the data in `page_params` is either
+        more highly dynamic than settings data, or
+        has more performance requirements than
+        simple settings data, or both.  Examples
+        include:
+
+            - tracking all users (we want to have
+              multiple Maps to find users, for example)
+            - tracking all streams
+            - tracking presence data
+            - tracking user groups and bots
+            - tracking recent PMs
+
+        Using stream data as an example, we use a
+        module called `stream_data` to actually track
+        all the info about the streams that a user
+        can know about.  We populate this module
+        with data from `page_params`, but thereafter
+        `stream_data.js` "owns" the stream data:
+
+            - other modules should ask `stream_data`
+              for stuff (and not go to `page_params`)
+            - when server events come in, they should
+              be processed by stream_data to update
+              its own data structures
+
+        To help enforce this paradigm, we do the
+        following:
+
+            - only pass `stream_data` what it needs
+              from `page_params`
+            - delete the reference to data owned by
+              `stream_data` in `page_params` itself
+    */
+
+    function pop_fields(...fields) {
+        const result = {};
+
+        for (const field of fields) {
+            result[field] = page_params[field];
+            delete page_params[field];
+        }
+
+        return result;
+    }
+
+    const bot_params = pop_fields(
+        'realm_bots'
+    );
+
+    const people_params = pop_fields(
+        'realm_users',
+        'realm_non_active_users',
+        'cross_realm_bots'
+    );
+
+    const presence_params = pop_fields(
+        'presences',
+        'initial_servertime'
+    );
+
+    const stream_data_params = pop_fields(
+        'subscriptions',
+        'unsubscribed',
+        'never_subscribed'
+    );
+
+    const user_groups_params = pop_fields(
+        'realm_user_groups'
+    );
+
+    const user_status_params = pop_fields(
+        'user_status'
+    );
+
     emojisets.initialize();
-    people.initialize();
+    people.initialize(page_params.user_id, people_params);
     scroll_bar.initialize();
     message_viewport.initialize();
     exports.initialize_kitchen_sink_stuff();
     echo.initialize();
     stream_color.initialize();
     stream_edit.initialize();
-    stream_data.initialize();
+    stream_data.initialize(stream_data_params);
     pm_conversations.recent.initialize();
     muting.initialize();
     subs.initialize();
@@ -320,13 +437,13 @@ exports.initialize_everything = function () {
         tab_bar.initialize();
     }
     server_events.initialize();
-    user_status.initialize();
+    user_status.initialize(user_status_params);
     compose_pm_pill.initialize();
     search_pill_widget.initialize();
     reload.initialize();
-    user_groups.initialize();
+    user_groups.initialize(user_groups_params);
     unread.initialize();
-    bot_data.initialize(); // Must happen after people.initialize()
+    bot_data.initialize(bot_params); // Must happen after people.initialize()
     message_fetch.initialize();
     message_scroll.initialize();
     emoji.initialize();
@@ -346,7 +463,7 @@ exports.initialize_everything = function () {
     hashchange.initialize();
     pointer.initialize();
     unread_ui.initialize();
-    presence.initialize();
+    presence.initialize(presence_params);
     activity.initialize();
     emoji_picker.initialize();
     compose_fade.initialize();

static/js/user_groups.js

@@ -84,12 +84,10 @@ exports.remove_members = function (user_group_id, user_ids) {
     }
 };
 
-exports.initialize = function () {
+exports.initialize = function (params) {
-    for (const user_group of page_params.realm_user_groups) {
+    for (const user_group of params.realm_user_groups) {
         exports.add(user_group);
     }
-
-    delete page_params.realm_user_groups; // We are the only consumer of this.
 };
 
 exports.is_user_group = function (item) {

static/js/user_status.js

@@ -56,8 +56,8 @@ exports.set_status_text = function (opts) {
     user_info.set(opts.user_id, opts.status_text);
 };
 
-exports.initialize = function () {
+exports.initialize = function (params) {
-    for (const [str_user_id, dct] of Object.entries(page_params.user_status)) {
+    for (const [str_user_id, dct] of Object.entries(params.user_status)) {
         // JSON does not allow integer keys, so we
         // convert them here.
         const user_id = parseInt(str_user_id, 10);
@@ -70,8 +70,6 @@ exports.initialize = function () {
             user_info.set(user_id, dct.status_text);
         }
     }
-
-    delete page_params.user_status;
 };
 
 window.user_status = exports;