From ff706e542d73743d053fbfa0a0cfc8925b41e5e9 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Wed, 10 Aug 2022 22:23:51 +0530 Subject: [PATCH 1/8] Throw ConnectionError instead of swallowing error --- src/platform/web/dom/request/fetch.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/platform/web/dom/request/fetch.js b/src/platform/web/dom/request/fetch.js index eb4caab6..5f6dcb65 100644 --- a/src/platform/web/dom/request/fetch.js +++ b/src/platform/web/dom/request/fetch.js @@ -119,10 +119,10 @@ export function createFetchRequest(createTimeout, serviceWorkerHandler) { body = await response.text(); } } catch (err) { - // some error pages return html instead of json, ignore error - if (!(err.name === "SyntaxError" && status >= 400)) { - throw err; + if (err.name === "SyntaxError" && status >= 400) { + throw new ConnectionError(`${method} ${url}: Failed to fetch JSON file!`); } + throw err; } return {status, body}; }, err => { From 27363b3f63082e44cdaaf0a86658f499d223bec1 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Wed, 10 Aug 2022 22:25:56 +0530 Subject: [PATCH 2/8] Throw and log errors if manifests cannot be loaded --- src/platform/web/theming/ThemeLoader.ts | 29 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/platform/web/theming/ThemeLoader.ts b/src/platform/web/theming/ThemeLoader.ts index be1bafc0..7519329d 100644 --- a/src/platform/web/theming/ThemeLoader.ts +++ b/src/platform/web/theming/ThemeLoader.ts @@ -14,12 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -import type {ILogItem} from "../../../logging/types"; -import type {Platform} from "../Platform.js"; import {RuntimeThemeParser} from "./parsers/RuntimeThemeParser"; -import type {Variant, ThemeInformation} from "./parsers/types"; import {ColorSchemePreference} from "./parsers/types"; import {BuiltThemeParser} from "./parsers/BuiltThemeParser"; +import type {Variant, ThemeInformation} from "./parsers/types"; +import type {ThemeManifest} from "../../types/theme"; +import type {ILogItem} from "../../../logging/types"; +import type {Platform} from "../Platform.js"; export class ThemeLoader { private _platform: Platform; @@ -32,21 +33,31 @@ export class ThemeLoader { async init(manifestLocations: string[], log?: ILogItem): Promise { await this._platform.logger.wrapOrRun(log, "ThemeLoader.init", async (log) => { - const results = await Promise.all( + let noManifestsAvailable = true; + const failedManifestLoads: string[] = []; + const results = await Promise.allSettled( manifestLocations.map(location => this._platform.request(location, { method: "GET", format: "json", cache: true, }).response()) ); const runtimeThemeParser = new RuntimeThemeParser(this._platform, this.preferredColorScheme); const builtThemeParser = new BuiltThemeParser(this.preferredColorScheme); const runtimeThemePromises: Promise[] = []; for (let i = 0; i < results.length; ++i) { - const { body } = results[i]; + const result = results[i]; + if (result.status === "rejected") { + console.error(`Failed to load manifest at ${manifestLocations[i]}, reason: ${result.reason}`); + log.log({ l: "Manifest fetch failed", location: manifestLocations[i], reason: result.reason }); + failedManifestLoads.push(manifestLocations[i]) + continue; + } + noManifestsAvailable = false; + const { body } = result.value; try { if (body.extends) { - const indexOfBaseManifest = results.findIndex(manifest => manifest.body.id === body.extends); + const indexOfBaseManifest = results.findIndex(result => "value" in result && result.value.body.id === body.extends); if (indexOfBaseManifest === -1) { throw new Error(`Base manifest for derived theme at ${manifestLocations[i]} not found!`); } - const {body: baseManifest} = results[indexOfBaseManifest]; + const { body: baseManifest } = (results[indexOfBaseManifest] as PromiseFulfilledResult<{ body: ThemeManifest }>).value; const baseManifestLocation = manifestLocations[indexOfBaseManifest]; const promise = runtimeThemeParser.parse(body, baseManifest, baseManifestLocation, log); runtimeThemePromises.push(promise); @@ -59,6 +70,10 @@ export class ThemeLoader { console.error(e); } } + if (noManifestsAvailable) { + // We need at least one working theme manifest! + throw new Error(`All configured theme manifests failed to load, the following were tried: ${failedManifestLoads.join(", ")}`); + } await Promise.all(runtimeThemePromises); this._themeMapping = { ...builtThemeParser.themeMapping, ...runtimeThemeParser.themeMapping }; Object.assign(this._themeMapping, builtThemeParser.themeMapping, runtimeThemeParser.themeMapping); From 2e12ce74b792e20b73d94915769aab6e3e6521d9 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Mon, 15 Aug 2022 17:23:27 +0530 Subject: [PATCH 3/8] Show parse errors in the UI as well --- src/platform/web/theming/ThemeLoader.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/platform/web/theming/ThemeLoader.ts b/src/platform/web/theming/ThemeLoader.ts index 7519329d..96293009 100644 --- a/src/platform/web/theming/ThemeLoader.ts +++ b/src/platform/web/theming/ThemeLoader.ts @@ -35,6 +35,7 @@ export class ThemeLoader { await this._platform.logger.wrapOrRun(log, "ThemeLoader.init", async (log) => { let noManifestsAvailable = true; const failedManifestLoads: string[] = []; + const parseErrors: string[] = []; const results = await Promise.allSettled( manifestLocations.map(location => this._platform.request(location, { method: "GET", format: "json", cache: true, }).response()) ); @@ -68,15 +69,19 @@ export class ThemeLoader { } catch(e) { console.error(e); + parseErrors.push(e.message); } } + await Promise.all(runtimeThemePromises); + this._themeMapping = { ...builtThemeParser.themeMapping, ...runtimeThemeParser.themeMapping }; if (noManifestsAvailable) { // We need at least one working theme manifest! throw new Error(`All configured theme manifests failed to load, the following were tried: ${failedManifestLoads.join(", ")}`); } - await Promise.all(runtimeThemePromises); - this._themeMapping = { ...builtThemeParser.themeMapping, ...runtimeThemeParser.themeMapping }; - Object.assign(this._themeMapping, builtThemeParser.themeMapping, runtimeThemeParser.themeMapping); + else if (Object.keys(this._themeMapping).length === 0 && parseErrors.length) { + // Something is wrong..., themeMapping is empty! + throw new Error(`Failed to parse theme manifests, the following errors were encountered: ${parseErrors.join(", ")}`); + } this._addDefaultThemeToMapping(log); log.log({ l: "Preferred colorscheme", scheme: this.preferredColorScheme === ColorSchemePreference.Dark ? "dark" : "light" }); log.log({ l: "Result", themeMapping: this._themeMapping }); From 7590c5540475d7bf0455bfb57217c0697741c76a Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Mon, 15 Aug 2022 22:28:40 +0530 Subject: [PATCH 4/8] Log error when loading css file fails --- src/platform/web/Platform.js | 38 +++++++++++++++++-------- src/platform/web/theming/ThemeLoader.ts | 6 ++-- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/platform/web/Platform.js b/src/platform/web/Platform.js index 15923a86..df24418c 100644 --- a/src/platform/web/Platform.js +++ b/src/platform/web/Platform.js @@ -192,7 +192,7 @@ export class Platform { await this._themeLoader?.init(manifests, log); const { themeName, themeVariant } = await this._themeLoader.getActiveTheme(); log.log({ l: "Active theme", name: themeName, variant: themeVariant }); - this._themeLoader.setTheme(themeName, themeVariant, log); + await this._themeLoader.setTheme(themeName, themeVariant, log); } }); } catch (err) { @@ -332,17 +332,31 @@ export class Platform { return this._themeLoader; } - replaceStylesheet(newPath) { - const head = document.querySelector("head"); - // remove default theme - document.querySelectorAll(".theme").forEach(e => e.remove()); - // add new theme - const styleTag = document.createElement("link"); - styleTag.href = newPath; - styleTag.rel = "stylesheet"; - styleTag.type = "text/css"; - styleTag.className = "theme"; - head.appendChild(styleTag); + async replaceStylesheet(newPath, log) { + await this.logger.wrapOrRun(log, { l: "replaceStylesheet", location: newPath, }, async (l) => { + let resolve; + const promise = new Promise(r => resolve = r); + const head = document.querySelector("head"); + // remove default theme + document.querySelectorAll(".theme").forEach(e => e.remove()); + // add new theme + const styleTag = document.createElement("link"); + styleTag.href = newPath; + styleTag.rel = "stylesheet"; + styleTag.type = "text/css"; + styleTag.className = "theme"; + styleTag.onerror = () => { + const error = new Error(`Failed to load stylesheet at ${newPath}`); + l.catch(error); + resolve(); + throw error + }; + styleTag.onload = () => { + resolve(); + }; + head.appendChild(styleTag); + await promise; + }); } get description() { diff --git a/src/platform/web/theming/ThemeLoader.ts b/src/platform/web/theming/ThemeLoader.ts index 96293009..d15108a2 100644 --- a/src/platform/web/theming/ThemeLoader.ts +++ b/src/platform/web/theming/ThemeLoader.ts @@ -88,8 +88,8 @@ export class ThemeLoader { }); } - setTheme(themeName: string, themeVariant?: "light" | "dark" | "default", log?: ILogItem) { - this._platform.logger.wrapOrRun(log, { l: "change theme", name: themeName, variant: themeVariant }, () => { + async setTheme(themeName: string, themeVariant?: "light" | "dark" | "default", log?: ILogItem) { + await this._platform.logger.wrapOrRun(log, { l: "change theme", name: themeName, variant: themeVariant }, async (l) => { let cssLocation: string, variables: Record; let themeDetails = this._themeMapping[themeName]; if ("id" in themeDetails) { @@ -103,7 +103,7 @@ export class ThemeLoader { cssLocation = themeDetails[themeVariant].cssLocation; variables = themeDetails[themeVariant].variables; } - this._platform.replaceStylesheet(cssLocation); + await this._platform.replaceStylesheet(cssLocation, l); if (variables) { log?.log({l: "Derived Theme", variables}); this._injectCSSVariables(variables); From 6335da09326d8ab6c95a6bad0a59048c98376bcf Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Mon, 15 Aug 2022 22:52:02 +0530 Subject: [PATCH 5/8] Throw error from outside log method This will show the error in the UI --- src/platform/web/Platform.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/platform/web/Platform.js b/src/platform/web/Platform.js index df24418c..3e6de0c3 100644 --- a/src/platform/web/Platform.js +++ b/src/platform/web/Platform.js @@ -333,8 +333,8 @@ export class Platform { } async replaceStylesheet(newPath, log) { - await this.logger.wrapOrRun(log, { l: "replaceStylesheet", location: newPath, }, async (l) => { - let resolve; + const error = await this.logger.wrapOrRun(log, { l: "replaceStylesheet", location: newPath, }, async (l) => { + let resolve, error; const promise = new Promise(r => resolve = r); const head = document.querySelector("head"); // remove default theme @@ -346,17 +346,20 @@ export class Platform { styleTag.type = "text/css"; styleTag.className = "theme"; styleTag.onerror = () => { - const error = new Error(`Failed to load stylesheet at ${newPath}`); + error = new Error(`Failed to load stylesheet from ${newPath}`); l.catch(error); resolve(); - throw error }; styleTag.onload = () => { resolve(); }; head.appendChild(styleTag); await promise; + return error; }); + if (error) { + throw error; + } } get description() { From 08f9edaf68ab15882f7a15cb2589e71695e86fd8 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Mon, 15 Aug 2022 22:58:26 +0530 Subject: [PATCH 6/8] Use Error LogLevel --- src/platform/web/theming/ThemeLoader.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform/web/theming/ThemeLoader.ts b/src/platform/web/theming/ThemeLoader.ts index d15108a2..6382dcdb 100644 --- a/src/platform/web/theming/ThemeLoader.ts +++ b/src/platform/web/theming/ThemeLoader.ts @@ -21,6 +21,7 @@ import type {Variant, ThemeInformation} from "./parsers/types"; import type {ThemeManifest} from "../../types/theme"; import type {ILogItem} from "../../../logging/types"; import type {Platform} from "../Platform.js"; +import {LogLevel} from "../../../logging/LogFilter"; export class ThemeLoader { private _platform: Platform; @@ -46,7 +47,7 @@ export class ThemeLoader { const result = results[i]; if (result.status === "rejected") { console.error(`Failed to load manifest at ${manifestLocations[i]}, reason: ${result.reason}`); - log.log({ l: "Manifest fetch failed", location: manifestLocations[i], reason: result.reason }); + log.log({ l: "Manifest fetch failed", location: manifestLocations[i], reason: result.reason }, LogLevel.Error); failedManifestLoads.push(manifestLocations[i]) continue; } From 5d63069f317fd3d553c7121e90537a8e88e2e88b Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Tue, 16 Aug 2022 14:32:18 +0530 Subject: [PATCH 7/8] Check status code instead of throwing error --- src/platform/web/dom/request/fetch.js | 7 ++++--- src/platform/web/theming/ThemeLoader.ts | 10 +++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/platform/web/dom/request/fetch.js b/src/platform/web/dom/request/fetch.js index 5f6dcb65..c2e2d4b7 100644 --- a/src/platform/web/dom/request/fetch.js +++ b/src/platform/web/dom/request/fetch.js @@ -119,10 +119,11 @@ export function createFetchRequest(createTimeout, serviceWorkerHandler) { body = await response.text(); } } catch (err) { - if (err.name === "SyntaxError" && status >= 400) { - throw new ConnectionError(`${method} ${url}: Failed to fetch JSON file!`); + // some error pages return html instead of json, ignore error + // detect these ignored errors from the response status + if (!(err.name === "SyntaxError" && status >= 400)) { + throw err; } - throw err; } return {status, body}; }, err => { diff --git a/src/platform/web/theming/ThemeLoader.ts b/src/platform/web/theming/ThemeLoader.ts index 6382dcdb..665c3a17 100644 --- a/src/platform/web/theming/ThemeLoader.ts +++ b/src/platform/web/theming/ThemeLoader.ts @@ -37,7 +37,7 @@ export class ThemeLoader { let noManifestsAvailable = true; const failedManifestLoads: string[] = []; const parseErrors: string[] = []; - const results = await Promise.allSettled( + const results = await Promise.all( manifestLocations.map(location => this._platform.request(location, { method: "GET", format: "json", cache: true, }).response()) ); const runtimeThemeParser = new RuntimeThemeParser(this._platform, this.preferredColorScheme); @@ -45,14 +45,14 @@ export class ThemeLoader { const runtimeThemePromises: Promise[] = []; for (let i = 0; i < results.length; ++i) { const result = results[i]; - if (result.status === "rejected") { - console.error(`Failed to load manifest at ${manifestLocations[i]}, reason: ${result.reason}`); - log.log({ l: "Manifest fetch failed", location: manifestLocations[i], reason: result.reason }, LogLevel.Error); + const { status, body } = result; + if (!(status >= 200 && status <= 299)) { + console.error(`Failed to load manifest at ${manifestLocations[i]}, status: ${status}`); + log.log({ l: "Manifest fetch failed", location: manifestLocations[i], status }, LogLevel.Error); failedManifestLoads.push(manifestLocations[i]) continue; } noManifestsAvailable = false; - const { body } = result.value; try { if (body.extends) { const indexOfBaseManifest = results.findIndex(result => "value" in result && result.value.body.id === body.extends); From 86fec8bf0e0488c89825c81f7fd2a7b25cc5c80e Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Fri, 19 Aug 2022 18:22:37 +0530 Subject: [PATCH 8/8] Make code more readable --- src/platform/web/Platform.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/platform/web/Platform.js b/src/platform/web/Platform.js index 3e6de0c3..29a83e1f 100644 --- a/src/platform/web/Platform.js +++ b/src/platform/web/Platform.js @@ -334,8 +334,7 @@ export class Platform { async replaceStylesheet(newPath, log) { const error = await this.logger.wrapOrRun(log, { l: "replaceStylesheet", location: newPath, }, async (l) => { - let resolve, error; - const promise = new Promise(r => resolve = r); + let error; const head = document.querySelector("head"); // remove default theme document.querySelectorAll(".theme").forEach(e => e.remove()); @@ -345,14 +344,16 @@ export class Platform { styleTag.rel = "stylesheet"; styleTag.type = "text/css"; styleTag.className = "theme"; - styleTag.onerror = () => { - error = new Error(`Failed to load stylesheet from ${newPath}`); - l.catch(error); - resolve(); - }; - styleTag.onload = () => { - resolve(); - }; + const promise = new Promise(resolve => { + styleTag.onerror = () => { + error = new Error(`Failed to load stylesheet from ${newPath}`); + l.catch(error); + resolve(); + }; + styleTag.onload = () => { + resolve(); + }; + }); head.appendChild(styleTag); await promise; return error;