refactor: Extract browser_history module.

This mainly extracts a new module called
browser_history. It has much fewer dependencies
than hashchange.js, so any modules that just
need the smaller API from browser_history now
have fewer transitive dependencies.

Here are some details:
    * Move is_overlay_hash to hash_util.
    * Rename hashchange.update_browser_history to
      brower_history.update
    * Move go_to_location verbatim.
    * Remove unused argument for exit_overlay.
    * Introduce helper functions:
        * old_hash()
        * set_hash_before_overlay()
        * save_old_hash()

We now have 100% line coverage on the extracted
code.
This commit is contained in:
Steve Howell
2021-03-22 15:09:12 +00:00
committed by Tim Abbott
parent 67a487db79
commit 746cc9e1f6
23 changed files with 194 additions and 126 deletions

View File

@@ -54,12 +54,12 @@ all of this (would be a good project to add them to the
[Puppeteer suite][testing-with-puppeteer]) and there's enough complexity
that it's easy to accidentally break something.
The main external API is below:
The main external API lives in `static/js/browser_history.js`:
* `hashchange.update_browser_history` is used to update the browser
* `browser_history.update` is used to update the browser
history, and it should be called when the app code is taking care
of updating the UI directly
* `hashchange.go_to_location` is used when you want the `hashchange`
* `browser_history.go_to_location` is used when you want the `hashchange`
module to actually dispatch building the next page
Internally you have these functions:

View File

@@ -0,0 +1,51 @@
"use strict";
const {strict: assert} = require("assert");
const {set_global, zrequire} = require("../zjsunit/namespace");
const {run_test} = require("../zjsunit/test");
const blueslip = require("../zjsunit/zblueslip");
const browser_history = zrequire("browser_history");
const location = set_global("location", {
hash: "bogus",
});
function test(label, f) {
run_test(label, (override) => {
location.hash = "bogus";
browser_history.clear_for_testing();
f(override);
});
}
test("basics", () => {
const hash1 = "#settings/your-account";
const hash2 = "#narrow/is/private";
browser_history.go_to_location(hash1);
assert.equal(location.hash, hash1);
browser_history.update(hash2);
assert.equal(location.hash, hash2);
assert.equal(browser_history.old_hash(), hash1);
const was_internal_change = browser_history.save_old_hash();
assert(was_internal_change);
assert.equal(browser_history.old_hash(), hash2);
});
test("update with same hash", () => {
const hash = "#keyboard-shortcuts";
browser_history.update(hash);
assert.equal(location.hash, hash);
browser_history.update(hash);
assert.equal(location.hash, hash);
});
test("error for bad hashes", () => {
const hash = "bogus";
blueslip.expect("error", "programming error: prefix hashes with #: bogus");
browser_history.update(hash);
});

View File

@@ -36,6 +36,7 @@ mock_esm("../../static/js/top_left_corner", {
});
set_global("favicon", {});
const browser_history = zrequire("browser_history");
const people = zrequire("people");
const hash_util = zrequire("hash_util");
const hashchange = zrequire("hashchange");
@@ -173,7 +174,7 @@ run_test("hash_interactions", (override) => {
window.location.hash = "#all_messages";
hashchange.clear_for_testing();
browser_history.clear_for_testing();
hashchange.initialize();
helper.assert_events([
[overlays, "close_for_hash_change"],
@@ -275,15 +276,10 @@ run_test("hash_interactions", (override) => {
[admin, "launch"],
]);
let called_back;
helper.clear_events();
hashchange.exit_overlay(() => {
called_back = true;
});
browser_history.exit_overlay();
helper.assert_events([[ui_util, "blur_active_element"]]);
assert(called_back);
});
run_test("save_narrow", (override) => {

View File

@@ -42,6 +42,7 @@ const page_params = set_global("page_params", {});
set_global("document", "document-stub");
mock_cjs("jquery", $);
const browser_history = mock_esm("../../static/js/browser_history");
const compose_actions = mock_esm("../../static/js/compose_actions");
const condense = mock_esm("../../static/js/condense");
const drafts = mock_esm("../../static/js/drafts");
@@ -51,9 +52,6 @@ const emoji_picker = mock_esm("../../static/js/emoji_picker", {
const gear_menu = mock_esm("../../static/js/gear_menu", {
is_open: () => false,
});
const hashchange = mock_esm("../../static/js/hashchange", {
in_recent_topics_hash: () => false,
});
const lightbox = mock_esm("../../static/js/lightbox");
const list_util = mock_esm("../../static/js/list_util");
const message_edit = mock_esm("../../static/js/message_edit");
@@ -79,6 +77,10 @@ const search = mock_esm("../../static/js/search");
const stream_list = mock_esm("../../static/js/stream_list");
const subs = mock_esm("../../static/js/subs");
mock_esm("../../static/js/hashchange", {
in_recent_topics_hash: () => false,
});
mock_esm("../../static/js/stream_popover", {
stream_popped: () => false,
topic_popped: () => false,
@@ -293,7 +295,7 @@ run_test("streams", (override) => {
});
run_test("basic mappings", () => {
assert_mapping("?", hashchange, "go_to_location");
assert_mapping("?", browser_history, "go_to_location");
assert_mapping("/", search, "initiate_search");
assert_mapping("w", activity, "initiate_search");
assert_mapping("q", stream_list, "initiate_search");

View File

@@ -17,11 +17,11 @@ const ui = mock_esm("../../static/js/ui", {
get_scroll_element: noop,
});
mock_esm("../../static/js/browser_history", {update: noop});
mock_esm("../../static/js/hash_util", {
stream_edit_uri: noop,
by_stream_uri: noop,
});
mock_esm("../../static/js/hashchange", {update_browser_history: noop});
mock_esm("../../static/js/list_widget", {
create: () => ({init: noop}),
});

View File

@@ -0,0 +1,68 @@
import * as blueslip from "./blueslip";
import * as hash_util from "./hash_util";
import * as ui_util from "./ui_util";
const state = {
is_internal_change: false,
hash_before_overlay: null,
old_hash: typeof window !== "undefined" ? window.location.hash : "#",
};
export function clear_for_testing() {
state.is_internal_change = false;
state.hash_before_overlay = null;
state.old_hash = "#";
}
export function old_hash() {
return state.old_hash;
}
export function set_hash_before_overlay(hash) {
state.hash_before_overlay = hash;
}
export function save_old_hash() {
state.old_hash = window.location.hash;
const was_internal_change = state.is_internal_change;
state.is_internal_change = false;
return was_internal_change;
}
export function update(new_hash) {
const old_hash = window.location.hash;
if (!new_hash.startsWith("#")) {
blueslip.error("programming error: prefix hashes with #: " + new_hash);
return;
}
if (old_hash === new_hash) {
// If somebody is calling us with the same hash we already have, it's
// probably harmless, and we just ignore it. But it could be a symptom
// of disorganized code that's prone to an infinite loop of repeatedly
// assigning the same hash.
blueslip.info("ignoring probably-harmless call to browser_history.update: " + new_hash);
return;
}
state.old_hash = old_hash;
state.is_internal_change = true;
window.location.hash = new_hash;
}
export function exit_overlay() {
if (hash_util.is_overlay_hash(window.location.hash)) {
ui_util.blur_active_element();
const new_hash = state.hash_before_overlay || "#";
update(new_hash);
}
}
export function go_to_location(hash) {
// Call this function when you WANT the hashchanged
// function to run.
window.location.hash = hash;
}

View File

@@ -9,6 +9,7 @@ import render_buddy_list_tooltip_content from "../templates/buddy_list_tooltip_c
import * as activity from "./activity";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as buddy_data from "./buddy_data";
import * as channel from "./channel";
import * as compose from "./compose";
@@ -16,7 +17,6 @@ import * as compose_actions from "./compose_actions";
import * as compose_state from "./compose_state";
import * as emoji_picker from "./emoji_picker";
import * as hash_util from "./hash_util";
import * as hashchange from "./hashchange";
import * as hotspots from "./hotspots";
import * as message_edit from "./message_edit";
import * as message_edit_history from "./message_edit_history";
@@ -282,7 +282,7 @@ export function initialize() {
// so we re-encode the hash.
const stream_id = Number.parseInt($(this).attr("data-stream-id"), 10);
if (stream_id) {
hashchange.go_to_location(hash_util.by_stream_uri(stream_id));
browser_history.go_to_location(hash_util.by_stream_uri(stream_id));
return;
}
window.location.href = $(this).attr("href");
@@ -716,7 +716,7 @@ export function initialize() {
$("body").on("click", "[data-overlay-trigger]", function () {
const target = $(this).attr("data-overlay-trigger");
hashchange.go_to_location(target);
browser_history.go_to_location(target);
});
function handle_compose_click(e) {
@@ -752,7 +752,7 @@ export function initialize() {
$("#streams_inline_cog").on("click", (e) => {
e.stopPropagation();
hashchange.go_to_location("streams/subscribed");
browser_history.go_to_location("streams/subscribed");
});
$("#streams_filter_icon").on("click", (e) => {

View File

@@ -1,6 +1,6 @@
import $ from "jquery";
import * as hashchange from "./hashchange";
import * as browser_history from "./browser_history";
if (window.electron_bridge !== undefined) {
window.electron_bridge.on_event("logout", () => {
@@ -8,10 +8,10 @@ if (window.electron_bridge !== undefined) {
});
window.electron_bridge.on_event("show-keyboard-shortcuts", () => {
hashchange.go_to_location("keyboard-shortcuts");
browser_history.go_to_location("keyboard-shortcuts");
});
window.electron_bridge.on_event("show-notification-settings", () => {
hashchange.go_to_location("settings/notifications");
browser_history.go_to_location("settings/notifications");
});
}

View File

@@ -5,12 +5,12 @@ import $ from "jquery";
import render_draft_table_body from "../templates/draft_table_body.hbs";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as compose from "./compose";
import * as compose_actions from "./compose_actions";
import * as compose_fade from "./compose_fade";
import * as compose_state from "./compose_state";
import * as compose_ui from "./compose_ui";
import * as hashchange from "./hashchange";
import {localstorage} from "./localstorage";
import * as markdown from "./markdown";
import * as narrow from "./narrow";
@@ -527,7 +527,7 @@ export function open_overlay() {
name: "drafts",
overlay: $("#draft_overlay"),
on_close() {
hashchange.exit_overlay();
browser_history.exit_overlay();
},
});
}

View File

@@ -70,7 +70,7 @@ The "info:" items use our info overlay system
in static/js/info_overlay.js. They are dispatched
using a click handler in static/js/click_handlers.js.
The click handler uses "[data-overlay-trigger]" as
the selector and then calls hash_change.go_to_location.
the selector and then calls browser_history.go_to_location.
*/

View File

@@ -209,3 +209,20 @@ export function parse_narrow(hash) {
}
return operators;
}
export function is_overlay_hash(hash) {
// Hash changes within this list are overlays and should not unnarrow (etc.)
const overlay_list = [
"streams",
"drafts",
"settings",
"organization",
"invite",
"keyboard-shortcuts",
"message-formatting",
"search-operators",
];
const main_hash = get_hash_category(hash);
return overlay_list.includes(main_hash);
}

View File

@@ -2,6 +2,7 @@ import $ from "jquery";
import * as admin from "./admin";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as drafts from "./drafts";
import * as floating_recipient_bar from "./floating_recipient_bar";
import * as hash_util from "./hash_util";
@@ -94,35 +95,6 @@ function activate_home_tab() {
setTimeout(navigate.maybe_scroll_to_selected, 0);
}
const state = {
is_internal_change: false,
hash_before_overlay: null,
old_hash: typeof window !== "undefined" ? window.location.hash : "#",
};
export function clear_for_testing() {
state.is_internal_change = false;
state.hash_before_overlay = null;
state.old_hash = "#";
}
function is_overlay_hash(hash) {
// Hash changes within this list are overlays and should not unnarrow (etc.)
const overlay_list = [
"streams",
"drafts",
"settings",
"organization",
"invite",
"keyboard-shortcuts",
"message-formatting",
"search-operators",
];
const main_hash = hash_util.get_hash_category(hash);
return overlay_list.includes(main_hash);
}
export function show_default_view() {
window.location.hash = page_params.default_view;
}
@@ -199,7 +171,7 @@ function do_hashchange_overlay(old_hash) {
const old_base = hash_util.get_hash_category(old_hash);
const section = hash_util.get_hash_section(window.location.hash);
const coming_from_overlay = is_overlay_hash(old_hash || "#");
const coming_from_overlay = hash_util.is_overlay_hash(old_hash || "#");
// Start by handling the specific case of going
// from something like streams/all to streams_subscribed.
@@ -248,7 +220,7 @@ function do_hashchange_overlay(old_hash) {
// NORMAL FLOW: basically, launch the overlay:
if (!coming_from_overlay) {
state.hash_before_overlay = old_hash;
browser_history.set_hash_before_overlay(old_hash);
}
if (base === "streams") {
@@ -293,15 +265,15 @@ function do_hashchange_overlay(old_hash) {
}
function hashchanged(from_reload, e) {
const old_hash = e && (e.oldURL ? new URL(e.oldURL).hash : state.old_hash);
state.old_hash = window.location.hash;
const old_hash = e && (e.oldURL ? new URL(e.oldURL).hash : browser_history.old_hash());
if (state.is_internal_change) {
state.is_internal_change = false;
const was_internal_change = browser_history.save_old_hash();
if (was_internal_change) {
return undefined;
}
if (is_overlay_hash(window.location.hash)) {
if (hash_util.is_overlay_hash(window.location.hash)) {
do_hashchange_overlay(old_hash);
return undefined;
}
@@ -314,28 +286,6 @@ function hashchanged(from_reload, e) {
return ret;
}
export function update_browser_history(new_hash) {
const old_hash = window.location.hash;
if (!new_hash.startsWith("#")) {
blueslip.error("programming error: prefix hashes with #: " + new_hash);
return;
}
if (old_hash === new_hash) {
// If somebody is calling us with the same hash we already have, it's
// probably harmless, and we just ignore it. But it could be a symptom
// of disorganized code that's prone to an infinite loop of repeatedly
// assigning the same hash.
blueslip.info("ignoring probably-harmless call to update_browser_history: " + new_hash);
return;
}
state.old_hash = old_hash;
state.is_internal_change = true;
window.location.hash = new_hash;
}
export function replace_hash(hash) {
if (!window.history.replaceState) {
// We may have strange behavior with the back button.
@@ -347,26 +297,9 @@ export function replace_hash(hash) {
window.history.replaceState(null, null, url);
}
export function go_to_location(hash) {
// Call this function when you WANT the hashchanged
// function to run.
window.location.hash = hash;
}
export function initialize() {
$(window).on("hashchange", (e) => {
hashchanged(false, e.originalEvent);
});
hashchanged(true);
}
export function exit_overlay(callback) {
if (is_overlay_hash(window.location.hash)) {
ui_util.blur_active_element();
const new_hash = state.hash_before_overlay || "#";
update_browser_history(new_hash);
if (typeof callback === "function") {
callback();
}
}
}

View File

@@ -3,6 +3,7 @@ import $ from "jquery";
import * as emoji from "../shared/js/emoji";
import * as activity from "./activity";
import * as browser_history from "./browser_history";
import * as common from "./common";
import * as compose from "./compose";
import * as compose_actions from "./compose_actions";
@@ -734,7 +735,7 @@ export function process_hotkey(e, hotkey) {
gear_menu.open();
return true;
case "show_shortcuts": // Show keyboard shortcuts page
hashchange.go_to_location("keyboard-shortcuts");
browser_history.go_to_location("keyboard-shortcuts");
return true;
case "stream_cycle_backward":
narrow.stream_cycle_backward();
@@ -749,10 +750,10 @@ export function process_hotkey(e, hotkey) {
narrow.narrow_to_next_pm_string();
return true;
case "open_recent_topics":
hashchange.go_to_location("#recent_topics");
browser_history.go_to_location("#recent_topics");
return true;
case "all_messages":
hashchange.go_to_location("#all_messages");
browser_history.go_to_location("#all_messages");
return true;
}

View File

@@ -2,9 +2,9 @@ import $ from "jquery";
import render_markdown_help from "../templates/markdown_help.hbs";
import * as browser_history from "./browser_history";
import * as common from "./common";
import * as components from "./components";
import * as hashchange from "./hashchange";
import * as keydown_util from "./keydown_util";
import * as markdown from "./markdown";
import * as overlays from "./overlays";
@@ -204,7 +204,7 @@ export function show(target) {
name: "informationalOverlays",
overlay,
on_close() {
hashchange.exit_overlay();
browser_history.exit_overlay();
},
});
}

View File

@@ -7,9 +7,9 @@ import render_invitation_failed_error from "../templates/invitation_failed_error
import render_invite_subscription from "../templates/invite_subscription.hbs";
import render_settings_dev_env_email_access from "../templates/settings/dev_env_email_access.hbs";
import * as browser_history from "./browser_history";
import * as channel from "./channel";
import * as common from "./common";
import * as hashchange from "./hashchange";
import * as overlays from "./overlays";
import * as stream_data from "./stream_data";
import * as ui from "./ui";
@@ -169,7 +169,7 @@ export function launch() {
name: "invite",
overlay: $("#invite-user"),
on_close() {
hashchange.exit_overlay();
browser_history.exit_overlay();
},
});

View File

@@ -808,7 +808,7 @@ function handle_post_narrow_deactivate_processes() {
export function deactivate(coming_from_recent_topics = false) {
// NOTE: Never call this function independently,
// always use hashchange.go_to_location("#all_messages") to
// always use browser_history.go_to_location("#all_messages") to
// activate All message narrow.
/*
Switches current_msg_list from narrowed_msg_list to

View File

@@ -1,7 +1,7 @@
import $ from "jquery";
import * as blueslip from "./blueslip";
import * as hashchange from "./hashchange";
import * as browser_history from "./browser_history";
import * as popovers from "./popovers";
let active_overlay;
@@ -224,7 +224,7 @@ export function open_settings() {
name: "settings",
overlay: $("#settings_overlay_container"),
on_close() {
hashchange.exit_overlay();
browser_history.exit_overlay();
},
});
}

View File

@@ -1,6 +1,6 @@
import $ from "jquery";
import * as hashchange from "./hashchange";
import * as browser_history from "./browser_history";
import * as search_pill from "./search_pill";
export let widget;
@@ -14,7 +14,7 @@ export function initialize() {
widget.onPillRemove(() => {
if (widget.items().length === 0) {
hashchange.go_to_location("");
browser_history.go_to_location("");
}
});

View File

@@ -1,6 +1,6 @@
import $ from "jquery";
import * as hashchange from "./hashchange";
import * as browser_history from "./browser_history";
import * as keydown_util from "./keydown_util";
import * as popovers from "./popovers";
import * as settings from "./settings";
@@ -114,7 +114,7 @@ export class SettingsPanelMenu {
this.curr_li.addClass("active");
const settings_section_hash = "#" + this.hash_prefix + section;
hashchange.update_browser_history(settings_section_hash);
browser_history.update(settings_section_hash);
$(".settings-section").removeClass("show");

View File

@@ -7,9 +7,9 @@ import render_subscription_settings from "../templates/subscription_settings.hbs
import render_subscription_stream_privacy_modal from "../templates/subscription_stream_privacy_modal.hbs";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as channel from "./channel";
import * as hash_util from "./hash_util";
import * as hashchange from "./hashchange";
import * as input_pill from "./input_pill";
import * as ListWidget from "./list_widget";
import * as narrow_state from "./narrow_state";
@@ -34,7 +34,7 @@ export let pill_widget;
function setup_subscriptions_stream_hash(sub) {
const hash = hash_util.stream_edit_uri(sub);
hashchange.update_browser_history(hash);
browser_history.update(hash);
}
function compare_by_email(a, b) {
@@ -50,9 +50,9 @@ function compare_by_name(a, b) {
export function setup_subscriptions_tab_hash(tab_key_value) {
if (tab_key_value === "all-streams") {
hashchange.update_browser_history("#streams/all");
browser_history.update("#streams/all");
} else if (tab_key_value === "subscribed") {
hashchange.update_browser_history("#streams/subscribed");
browser_history.update("#streams/subscribed");
} else {
blueslip.debug("Unknown tab_key_value: " + tab_key_value);
}

View File

@@ -9,9 +9,9 @@ import render_topic_sidebar_actions from "../templates/topic_sidebar_actions.hbs
import render_unstar_messages_modal from "../templates/unstar_messages_modal.hbs";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as channel from "./channel";
import * as hash_util from "./hash_util";
import * as hashchange from "./hashchange";
import * as message_edit from "./message_edit";
import * as message_flags from "./message_flags";
import * as muting from "./muting";
@@ -400,7 +400,7 @@ export function register_stream_handlers() {
hide_stream_popover();
const stream_edit_hash = hash_util.stream_edit_uri(sub);
hashchange.go_to_location(stream_edit_hash);
browser_history.go_to_location(stream_edit_hash);
});
// Pin/unpin

View File

@@ -7,11 +7,11 @@ import render_subscription_table_body from "../templates/subscription_table_body
import render_subscriptions from "../templates/subscriptions.hbs";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as channel from "./channel";
import * as components from "./components";
import * as compose_state from "./compose_state";
import * as hash_util from "./hash_util";
import * as hashchange from "./hashchange";
import * as loading from "./loading";
import * as message_live_update from "./message_live_update";
import * as message_view_header from "./message_view_header";
@@ -745,7 +745,7 @@ export function launch(section) {
name: "subscriptions",
overlay: $("#subscription_overlay"),
on_close() {
hashchange.exit_overlay();
browser_history.exit_overlay();
},
});
change_state(section);
@@ -819,7 +819,7 @@ export function view_stream() {
if (row_data) {
const stream_narrow_hash =
"#narrow/stream/" + hash_util.encode_stream_name(row_data.object.name);
hashchange.go_to_location(stream_narrow_hash);
browser_history.go_to_location(stream_narrow_hash);
}
}
@@ -942,7 +942,7 @@ export function do_open_create_stream() {
export function open_create_stream() {
do_open_create_stream();
hashchange.update_browser_history("#streams/new");
browser_history.update("#streams/new");
}
export function sub_or_unsub(sub, stream_row) {

View File

@@ -2,10 +2,10 @@ import $ from "jquery";
import marked from "../third/marked/lib/marked";
import * as browser_history from "./browser_history";
import * as channel from "./channel";
import * as common from "./common";
import * as feedback_widget from "./feedback_widget";
import * as hashchange from "./hashchange";
import * as night_mode from "./night_mode";
import * as scroll_bar from "./scroll_bar";
@@ -189,7 +189,7 @@ export function process(message_content) {
}
if (content === "/settings") {
hashchange.go_to_location("settings/your-account");
browser_history.go_to_location("settings/your-account");
return true;
}