appsignal-nodejs icon indicating copy to clipboard operation
appsignal-nodejs copied to clipboard

Only report extension load errors on start

Open unflxw opened this issue 9 months ago • 4 comments

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 Client.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 avatar May 07 '24 20:05 unflxw

I'm okay with this change but you need to update the tests for this.

tombruijn avatar May 08 '24 08:05 tombruijn

@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 avatar May 08 '24 09:05 unflxw

:heavy_check_mark: All good!

New issue guide | Backlog management | Rules | Feedback

backlog-helper[bot] avatar May 08 '24 13:05 backlog-helper[bot]

@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.

unflxw avatar May 08 '24 15:05 unflxw

  • This Pull Request needs more reviews. @jeffkreeftmeijer @tombruijn @luismiramirez - (More info)

This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

backlog-helper[bot] avatar May 10 '24 08:05 backlog-helper[bot]

Worth it to add a changeset .

tombruijn avatar May 10 '24 13:05 tombruijn