From 083a39a159ed60fbbd615486b3e596e27b700b4e Mon Sep 17 00:00:00 2001 From: Noemi Date: Thu, 10 Feb 2022 16:41:48 +0100 Subject: [PATCH] Show extension load error on require Before this commit, any errors in loading the extension wrapper module wouldn't be shown if the `Extension` object wasn't started, and the `BaseClient` object would not attempt to start the extension if the extension wrapper wasn't loaded, meaning that errors in loading the extension wrapper were, in practice, never shown. This commit changes those errors to be emitted when the extension wrapper module is required, instead of when the `Extension` object is started. --- .../show-extension-load-errors-when-required.md | 6 ++++++ .../src/__tests__/extension.failure.test.ts | 16 ++++++++++++---- packages/nodejs/src/extension.ts | 14 +------------- packages/nodejs/src/extension_wrapper.ts | 9 ++++++--- 4 files changed, 25 insertions(+), 20 deletions(-) create mode 100644 packages/nodejs/.changesets/show-extension-load-errors-when-required.md diff --git a/packages/nodejs/.changesets/show-extension-load-errors-when-required.md b/packages/nodejs/.changesets/show-extension-load-errors-when-required.md new file mode 100644 index 00000000..6d71d673 --- /dev/null +++ b/packages/nodejs/.changesets/show-extension-load-errors-when-required.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +type: "fix" +--- + +Show errors in loading the extension when the `@appsignal/nodejs` module is required. diff --git a/packages/nodejs/src/__tests__/extension.failure.test.ts b/packages/nodejs/src/__tests__/extension.failure.test.ts index 2c7b8fb7..fdd98b24 100644 --- a/packages/nodejs/src/__tests__/extension.failure.test.ts +++ b/packages/nodejs/src/__tests__/extension.failure.test.ts @@ -11,18 +11,26 @@ describe("Extension", () => { ext.stop() }) + it("logs a warning when the module is required", () => { + const errorSpy = jest.spyOn(console, "error") + + jest.resetModules() + require("../extension") + + expect(errorSpy).toHaveBeenLastCalledWith( + "AppSignal extension not loaded. This could mean that your current " + + "environment isn't supported, or that another error has occurred." + ) + }) + it("is not loaded", () => { expect(Extension.isLoaded).toEqual(false) }) it("does not start the client", () => { - const warnSpy = jest.spyOn(console, "warn") expect(() => { ext.start() }).not.toThrow() - expect(warnSpy).toHaveBeenLastCalledWith( - "AppSignal extension not loaded. This could mean that your current environment isn't supported, or that another error has occurred." - ) }) it("does not error on stopping the client", () => { diff --git a/packages/nodejs/src/extension.ts b/packages/nodejs/src/extension.ts index 7562e3cf..96a080b2 100644 --- a/packages/nodejs/src/extension.ts +++ b/packages/nodejs/src/extension.ts @@ -16,19 +16,7 @@ export class Extension { * Starts the extension. */ public start() { - try { - extension.start() - } catch (e) { - if (e.message === "Extension module not loaded") { - console.warn( - "AppSignal extension not loaded. This could mean that your current environment isn't supported, or that another error has occurred." - ) - } else { - console.error( - `Failed to load AppSignal extension with error: ${e.message}. Please email us at support@appsignal.com for support.` - ) - } - } + extension.start() } /** diff --git a/packages/nodejs/src/extension_wrapper.ts b/packages/nodejs/src/extension_wrapper.ts index 44d16e5d..b4483bef 100644 --- a/packages/nodejs/src/extension_wrapper.ts +++ b/packages/nodejs/src/extension_wrapper.ts @@ -6,12 +6,15 @@ try { mod = require("../build/Release/extension.node") as ExtensionWrapper mod.isLoaded = true } catch (e) { + console.error( + "AppSignal extension not loaded. This could mean that your current " + + "environment isn't supported, or that another error has occurred." + ) + mod = { isLoaded: false, extension: { - start() { - throw new Error("Extension module not loaded") - }, + start() {}, stop() {}, diagnoseRaw() {}, runningInContainer() {}