Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocDiff: Cache remote content #441

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
91 changes: 58 additions & 33 deletions src/docdiff.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { ajv } from "./data-validation";
import styleSheet from "./docdiff.css";
import docdiffGeneralStyleSheet from "./docdiff.document.css";

Expand All @@ -17,7 +16,7 @@ import {
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
EVENT_READTHEDOCS_DOCDIFF_HIDE,
} from "./events";
import { html, nothing, LitElement } from "lit";
import { nothing, LitElement } from "lit";
import { default as objectPath } from "object-path";
import { hasQueryParam, docTool } from "./utils";

Expand Down Expand Up @@ -84,6 +83,7 @@ export class DocDiffElement extends LitElement {
this.injectStyles = true;

this.originalBody = null;
this.cachedRemoteContent = null;
}

loadConfig(config) {
Expand All @@ -103,7 +103,8 @@ export class DocDiffElement extends LitElement {

// Enable DocDiff if the URL parameter is present
if (hasQueryParam(DOCDIFF_URL_PARAM)) {
this.enableDocDiff();
event = new CustomEvent(EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW);
document.dispatchEvent(event);
}
}

Expand All @@ -129,42 +130,56 @@ export class DocDiffElement extends LitElement {
}

compare() {
fetch(this.config.addons.doc_diff.base_url)
.then((response) => {
if (!response.ok) {
throw new Error("Error downloading requested base URL.");
}
let promiseData;

return response.text();
})
if (this.cachedRemoteContent !== null) {
promiseData = Promise.resolve(this.cachedRemoteContent);
} else {
promiseData = fetch(this.config.addons.doc_diff.base_url).then(
(response) => {
if (!response.ok) {
throw new Error("Error downloading requested base URL.");
}
return response.text();
},
);
}

promiseData
.then((text) => {
const parser = new DOMParser();
const html_document = parser.parseFromString(text, "text/html");
const old_body = html_document.documentElement.querySelector(
this.rootSelector,
);
const new_body = document.querySelector(this.rootSelector);

if (old_body == null || new_body == null) {
throw new Error("Element not found in both documents.");
}

// After finding the root element, and diffing it, replace it in the DOM
// with the resulting visual diff elements instead.
const diffNode = visualDomDiff.visualDomDiff(
old_body,
new_body,
VISUAL_DIFF_OPTIONS,
);
new_body.replaceWith(diffNode.firstElementChild);
this.cachedRemoteContent = text;
this.performDiff(text);
})
.catch((error) => {
console.error(error);
});
}

// After finding the root element, and diffing it, replace it in the DOM
// with the resulting visual diff elements instead.
performDiff(remoteContent) {
const parser = new DOMParser();
const html_document = parser.parseFromString(remoteContent, "text/html");
const old_body = html_document.documentElement.querySelector(
this.rootSelector,
);
const new_body = document.querySelector(this.rootSelector);

if (old_body == null || new_body == null) {
throw new Error("Element not found in both documents.");
}

const diffNode = visualDomDiff.visualDomDiff(
old_body,
new_body,
VISUAL_DIFF_OPTIONS,
);
new_body.replaceWith(diffNode.firstElementChild);
}

enableDocDiff() {
if (this.config === null) {
if (this.config === null || this.enabled) {
console.debug("Ignoring enableDocDiff: it was already enabled");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is not correct when this.config === null. It will be confusing in that case since it won't be enabled because there is no config.

return null;
}

Expand All @@ -174,6 +189,11 @@ export class DocDiffElement extends LitElement {
}

disableDocDiff() {
if (!this.enabled) {
console.debug("Ignoring disableDocDiff: it was already disabled");
return null;
}

this.enabled = false;
document.querySelector(this.rootSelector).replaceWith(this.originalBody);
}
Expand All @@ -195,15 +215,20 @@ export class DocDiffElement extends LitElement {
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleShowDocDiff,
);

document.addEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleHideDocDiff,
);
}

disconnectedCallback() {
document.removeEventListener("keydown", this._handleShowDocDiff);
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleShowDocDiff,
);
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleHideDocDiff,
);
super.disconnectedCallback();
}
}
Expand Down
39 changes: 31 additions & 8 deletions src/hotkeys.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { ajv } from "./data-validation";
import { toString as keyboardEventToString } from "keyboard-event-to-string";

import { AddonBase } from "./utils";
import { html, nothing, LitElement } from "lit";
import { LitElement } from "lit";
import {
EVENT_READTHEDOCS_SEARCH_SHOW,
EVENT_READTHEDOCS_SEARCH_HIDE,
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
EVENT_READTHEDOCS_DOCDIFF_HIDE,
} from "./events";
Expand All @@ -26,7 +24,6 @@ export class HotKeysElement extends LitElement {
super();

this.config = null;
this.docDiffShow = false;
}

loadConfig(config) {
Expand All @@ -40,7 +37,7 @@ export class HotKeysElement extends LitElement {
this.config = config;

this.docDiffHotKeyEnabled = this.config.addons.hotkeys.doc_diff.enabled;
this.docDiffShowed = false;
this.docDiffEnabled = false;

this.searchHotKeyEnabled = this.config.addons.hotkeys.search.enabled;
}
Expand All @@ -51,6 +48,7 @@ export class HotKeysElement extends LitElement {
// Read more about these decisions at https://github.com/readthedocs/addons/issues/80

let event;

// DocDiff
if (
this.docDiffHotKeyEnabled &&
Expand All @@ -60,12 +58,13 @@ export class HotKeysElement extends LitElement {
document.activeElement.tagName !== "TEXTAREA" &&
document.activeElement.tagName !== "READTHEDOCS-SEARCH"
) {
if (this.docDiffShowed) {
if (this.docDiffEnabled) {
event = new CustomEvent(EVENT_READTHEDOCS_DOCDIFF_HIDE);
this.docDiffShowed = false;
} else {
event = new CustomEvent(EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW);
this.docDiffShowed = true;
}
if (event !== undefined) {
document.dispatchEvent(event);
Comment on lines +65 to +67
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove this from here. The event is already dispatched in line 82 and it also prevent its default.

}
}

Expand All @@ -89,12 +88,36 @@ export class HotKeysElement extends LitElement {
connectedCallback() {
super.connectedCallback();
document.addEventListener("keydown", this._handleKeydown);
document.addEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleDocDiffShow,
);
document.addEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleDocDiffHide,
);
}

disconnectedCallback() {
document.removeEventListener("keydown", this._handleKeydown);
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleDocDiffShow,
);
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleDocDiffHide,
);
super.disconnectedCallback();
}

_handleDocDiffShow = (event) => {
this.docDiffEnabled = true;
};

_handleDocDiffHide = (event) => {
this.docDiffEnabled = false;
};
}

export class HotKeysAddon extends AddonBase {
Expand Down