-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add no-sidecar Sentry SDK integration for overlay #509
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Okay this works but we need some persistent storage as the events don't survive across page loads or even a simple reload. |
Seemingly best way forward:
|
Why do we need to care about previously expired events? I'm not yet sure I understand why we'd need to store anything in IndexdDB. Here's what I'm currently thinking:
I guess the only advantage we'd get with storing events is that we can show events from past page loads. Which is probably not always what we/users want but I can see it being helpful. If that's the goal then let's do it but IMO, this is something we should handle within spotlight and independent of if the event came in from the sidecar or from the event target. |
whoops, sorry for the accidental close! |
This is the current state anyway. Sidecar stores all past events (and without expiry which we may wanna address in the future due to memory concerns), and replays them as the first thing when a connection is made. When a user goes through pages or reloads, the events persist. In my own testing, I was also surprised when they were missing. Regarding at which level this should be done, I agree it makes sense to move it to Spotlight core so I'll tie this in to the new manual event trigger system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Though I'm a bit concerned with the TS bump (see comments). Given 5.4 isn't that old yet, I suggest we think twice if we need to bump it. I think in this case, rewriting Promise.withResolvers
should be doable. If you still want to bump, we can of course also cut a major.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for making the changes!
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @spotlightjs/overlay@2.4.0 ### Minor Changes - Add no-sidecar Sentry SDK integration for Spotlight overlay ([#509](#509)) - Add direct event ingestion through `Spotlight.sendEvent('<content-type>', <event>)` to allow sending events without the ([#508](#508)) sidecar ### Patch Changes - Fix stacktraces are not reversed sometimes (appearing in wrong order) ([#513](#513)) ## @spotlightjs/astro@2.1.7 ### Patch Changes - Updated dependencies \[]: - @spotlightjs/spotlight@2.3.2 ## @spotlightjs/electron@1.1.7 ### Patch Changes - Updated dependencies \[[`4acbad0ac4e5dc5a466af13ab2de50169bf8867b`](4acbad0), [`50135855e46c67c44d960c0ce0c22e001abcf982`](5013585), [`365ab8a1c085cf52a6620c0b8438eac44967f70f`](365ab8a)]: - @spotlightjs/overlay@2.4.0 ## @spotlightjs/spotlight@2.3.2 ### Patch Changes - Updated dependencies \[[`4acbad0ac4e5dc5a466af13ab2de50169bf8867b`](4acbad0), [`50135855e46c67c44d960c0ce0c22e001abcf982`](5013585), [`365ab8a1c085cf52a6620c0b8438eac44967f70f`](365ab8a)]: - @spotlightjs/overlay@2.4.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Ref #133.