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

Drop some comments and logging #8073

Merged
merged 13 commits into from
Mar 29, 2024
2 changes: 0 additions & 2 deletions src/bricks/renderers/documentView/DocumentViewLazy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import React, { Suspense } from "react";
import { type DocumentViewProps } from "./DocumentViewProps";

// Dynamic import because documentView has a transitive dependency of react-shadow-root which assumed a proper
// `window` variable is present on module load. This isn't available on header generation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment related to "headers" generation. We don't have such restrictions anymore

const DocumentView = React.lazy(
async () =>
import(
Expand Down
22 changes: 10 additions & 12 deletions src/bricks/transformers/controlFlow/WithAsyncModVariable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,13 @@ export class WithAsyncModVariable extends TransformerABC {
);
}

const bodyPromise = runPipeline(body, {
key: "body",
counter: 0,
});

void bodyPromise
// eslint-disable-next-line promise/prefer-await-to-then -- not blocking
.then((data: JsonObject) => {
// Non-blocking async call
(async () => {
try {
const data = (await runPipeline(body, {
key: "body",
counter: 0,
})) as JsonObject;
Copy link
Contributor Author

@fregante fregante Mar 27, 2024

Choose a reason for hiding this comment

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

Async-await code is easier to follow, and shorter if we account for the eslint-disable comments.

Here for example it makes the unknown as JsonObject assertion visible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider making runPipeline accept a generic instead of casting?

Copy link
Contributor

@twschiller twschiller Mar 27, 2024

Choose a reason for hiding this comment

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

That would be OK for now (e.g., TReturn). This type is actually wrong, I think the correct type is JsonValue. It just doesn't matter in practice because we pass it around as-is

The future change toRunPipeline would be to make the type TReturn extends JsonValue

if (!isCurrentNonce()) {
return;
}
Expand All @@ -267,9 +266,7 @@ export class WithAsyncModVariable extends TransformerABC {
},
"put",
);
})
// eslint-disable-next-line promise/prefer-await-to-then -- not blocking
.catch((error) => {
} catch (error) {
if (!isCurrentNonce()) {
return;
}
Expand All @@ -287,7 +284,8 @@ export class WithAsyncModVariable extends TransformerABC {
},
"put",
);
});
}
})();

return {
requestId,
Expand Down
16 changes: 0 additions & 16 deletions src/contentScript/sidebarController.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ let modActivationPanelEntry: ModActivationPanelEntry | null = null;
* Attach the sidebar to the page if it's not already attached. Then re-renders all panels.
*/
export async function showSidebar(): Promise<void> {
console.debug("sidebarController:showSidebar");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These logs are super old, we should try to remove old logs on parts of the extension that are stable and are no longer monitoring.

reportEvent(Events.SIDEBAR_SHOW);
if (isMV3() || isLoadedInIframe()) {
try {
Expand Down Expand Up @@ -178,10 +177,6 @@ export async function activateExtensionPanel(extensionId: UUID): Promise<void> {
* @see HIDE_SIDEBAR_EVENT_NAME
*/
export function hideSidebar(): void {
console.debug("sidebarController:hideSidebar", {
isSidebarFrameVisible: sidebarMv2.isSidebarFrameVisible(),
});

reportEvent(Events.SIDEBAR_HIDE);
sidebarMv2.removeSidebarFrame();
window.dispatchEvent(new CustomEvent(HIDE_SIDEBAR_EVENT_NAME));
Expand All @@ -208,14 +203,8 @@ export async function updateSidebar(
export async function renderPanelsIfVisible(): Promise<void> {
expectContext("contentScript");

console.debug("sidebarController:renderPanelsIfVisible");

if (await isSidePanelOpen()) {
void sidebarInThisTab.renderPanels(getTimedSequence(), panels);
} else {
console.debug(
"sidebarController:renderPanelsIfVisible: skipping renderPanels because the sidebar is not visible",
);
}
}

Expand Down Expand Up @@ -372,11 +361,6 @@ export function reservePanels(refs: ModComponentRef[]): void {
export function updateHeading(extensionId: UUID, heading: string): void {
const entry = panels.find((x) => x.extensionId === extensionId);

console.debug("sidebarController:updateHeading %s", extensionId, {
heading,
panel: entry,
});

if (entry) {
entry.heading = heading;
console.debug(
Expand Down
13 changes: 1 addition & 12 deletions src/contentScript/sidebarDomControllerLite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ import { uuidv4 } from "@/types/helpers";
export const SIDEBAR_WIDTH_CSS_PROPERTY = "--pb-sidebar-width";
const ORIGINAL_MARGIN_CSS_PROPERTY = "--pb-original-margin-right";

// Use ? because it's not defined during header generation. But otherwise it will always be defined.
// eslint-disable-next-line local-rules/persistBackgroundData -- Static
const html: HTMLElement = globalThis.document?.documentElement;
const html: HTMLElement = globalThis.document.documentElement;
const SIDEBAR_WIDTH_PX = 400;

function storeOriginalCSSOnce() {
Expand Down Expand Up @@ -79,11 +78,6 @@ export function isSidebarFrameVisible(): boolean {
/** Removes the element; Returns false if no element was found */
export function removeSidebarFrame(): boolean {
const sidebar = getSidebarElement();

console.debug("sidebarDomControllerLite:removeSidebarFrame", {
isSidebarFrameVisible: Boolean(sidebar),
});

if (sidebar) {
sidebar.remove();
setSidebarWidth(0);
Expand All @@ -94,12 +88,7 @@ export function removeSidebarFrame(): boolean {

/** Inserts the element; Returns false if it already existed */
export function insertSidebarFrame(): boolean {
console.debug("sidebarDomControllerLite:insertSidebarFrame", {
isSidebarFrameVisible: isSidebarFrameVisible(),
});

if (isSidebarFrameVisible()) {
console.debug("insertSidebarFrame: sidebar frame already exists");
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/pageEditor/PanelContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const PanelContent: React.FC = () => {
return () => {
navigationEvent.remove(onNavigation);
};
}, [navigationEvent, onNavigation]);
}, [onNavigation]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exhaustive-deps rule was warning on this line. navigationEvent never changes because it's "global"


useEffect(() => {
// Automatically connect on load
Expand Down
1 change: 0 additions & 1 deletion src/pageEditor/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
isCurrentTopFrame,
} from "@/pageEditor/context/connection";

// TODO: Migrate to useCurrentUrl()
async function onNavigation(target: Target): Promise<void> {
if (isCurrentTopFrame(target)) {
updatePageEditor();
Expand Down
13 changes: 6 additions & 7 deletions src/starterBricks/tourController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,14 @@ export async function registerTour({
markTourStart(nonce, extension, { promise, abortController, context });

// Decorate the extension promise with tour tracking
const runPromise = promise
// eslint-disable-next-line promise/prefer-await-to-then -- avoid separate method
.then(() => {
const runPromise = (async () => {
try {
await promise;
markTourEnd(nonce, { context });
})
// eslint-disable-next-line promise/prefer-await-to-then -- avoid separate method
.catch((error) => {
} catch (error) {
markTourEnd(nonce, { error, context });
});
}
})();

return {
nonce,
Expand Down
5 changes: 2 additions & 3 deletions src/types/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,8 @@ export function validateTimedSequence(string: string): TimedSequence {
* Return a random v4 UUID.
*/
export function uuidv4(): UUID {
// Use uuidv4 from uuid package instead of crypto.randomUUID because randomUUID is not available in insecure contexts.
// This is safe for content scripts because they're in a separate JS context from the host page.
// https://developer.mozilla.org/en-US/docs/Web/API/crypto_property
// Use `uuid` package instead of `crypto.randomUUID()` because the latter isn't available on HTTP-non-S pages
// https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
return v4() as UUID;
}

Expand Down
Loading