JulianMar / nuxt-bugsnag

Bugsnag module for nuxt.js
MIT License
49 stars 18 forks source link

Avoid caching to global context from plugin #31

Closed pi0 closed 2 years ago

pi0 commented 2 years ago

Hi. Readding through your module, i noticed that we are caching client instance from within the plugin to a global shared context (https://github.com/JulianMar/nuxt-bugsnag/blob/485537c256b7db87d0b53d1e9f809eab74f06e25/src/runtime/plugin.ts#L18) Nuxt plugins are a function to avoid sharing global state. If you really expect a global shared instance, why not directly initialize the client outside of the function body? this way if initially there are two parallel requests, we don't instantiate the client twice in a race condition as well.

JulianMar commented 2 years ago

Hey, thanks for your feedback!

I used this global state so I don't have to initalize bugsnag multiple times. As far as I understand it, the plugin will be called once on the server and once on the client, and each time it will be pushed to the nuxt context. Do you know a way to handle the case where it would be pushed multiple times?

Your Idea with creating the shared state outside of the function only kinda works. I need the options from the nuxt context to start the bugsnag plugin and I didn't find a way to access the settings outside of the plugin function.

pi0 commented 2 years ago

Indeed runtime config is available inside the context since depends on this context. In the future versions, we might even make it mutatable per request.

Probably the proper way would be to provide the logger instance by the plugin on each request.

and each time it will be pushed to the nuxt context.

Whatever inside the function body, yes. But outside variables are shared between requests.

Do you know a way to handle the case where it would be pushed multiple times?

It wouldn't be pushed multiple times but imaging when logger is not initially set and two parallel requests come. They both call async plugin and both executations, try to initialize same global variable. However it is probably no matter considering one instance will be garbage collected and there are no heavy operations for initialization, such pattern is not a good practice. Runtime config is only valid within it's context.

JulianMar commented 2 years ago

Okay I tried your approach and it looked good at first. After some time I noticed there are some errors in the console. [bugsnag] Bugsnag.start() was called more than once. Ignoring. This is cause because Bugsnag keeps it's own global state. https://github.com/bugsnag/bugsnag-js/issues/1638#issue-1095899696

I added their solution and called it a day. Seems to work good enough for me

Nuxt Bridge support was added aswell, so I can update it in the modules repo :) I noticed some problems with builds on bridge so I didn't add the support for sourcemaps, but will be added if it gets a bit more stable!

Thank you for your support and awesome work with nuxt 3! It was a joy developing a module for it. The hooks are awesome!