Only report extension load errors on start #1051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm okay with this change but you need to update the tests for this. |
|
@tombruijn Yeah, the tests are correct to be failing -- now the message is not emitted at all. I'll see what I can do to fix it. |
unflxw
force-pushed
the
only-report-extension-load-errors-on-start
branch
2 times, most recently
from
2c0b1ce to
b297d9a
Compare
|
✔️ All good! |
|
@tombruijn It's been fixed now -- but I've also changed a lot of other stuff in the process. I couldn't understand what the right place to emit these warnings in the client initialisation was, so I ended up rearranging it significantly to figure it out. |
This is a message from the daily scheduled checks. |
tombruijn
approved these changes
May 10, 2024
|
Worth it to add a changeset . |
Before this change, extension load errors would be reported whenever `@appsignal/nodejs` or the `client.ts` file was imported. Now they will only be reported whenever `Client` fails to initialise. This fixes an issue where the extension load errors would be reported when running `npm install` in the `appsignal-nodejs` folder. Rearrange the way that the client is initialised. Remove the `client.start()` method, as starting is done automatically during initialisation. Use early returns to clearly distinguish the client initialisation failure scenarios. Emit distinct error messages for different configuration-related failures. Change the `this.#isActive` method to reflect whether the client was actually initialised, rather than whether several factors in the configuration should mean that the client was initialised.
unflxw
force-pushed
the
only-report-extension-load-errors-on-start
branch
from
b297d9a to
6bf596c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, extension load errors would be reported whenever
@appsignal/nodejsor theclient.tsfile was imported. Now theywill only be reported whenever
Clientfails to initialise.This fixes an issue where the extension load errors would be reported
when running
npm installin theappsignal-nodejsfolder.Rearrange the way that the client is initialised. Remove the
client.start()method, as starting is done automatically duringinitialisation. Use early returns to clearly distinguish the client
initialisation failure scenarios. Emit distinct error messages for
different configuration-related failures.
Change the
Client.isActivemethod to reflect whether the clientwas actually initialised, rather than whether several factors in
the configuration should mean that the client was initialised.
This is an improvement to #960, as AppSignal will no longer show an error about the extension not loading when
activeis set tofalse. It would still be nice to have a dedicated, non-scary Windows error.