-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(vue): Add Pinia plugin #13841
feat(vue): Add Pinia plugin #13841
Conversation
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
23d8d11
to
6895af0
Compare
6895af0
to
1fa274a
Compare
size-limit report 📦
|
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.
Thanks for adding this 🙌
@@ -16,6 +16,7 @@ | |||
}, | |||
"dependencies": { | |||
"@sentry/vue": "latest || *", | |||
"pinia": "^2.2.3", |
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.
Should we add pinia as a peer dependency as well with major v2?
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.
Sure, added pinia
as an optional peer dependency. 👍
packages/vue/src/pinia.ts
Outdated
const options = client && client.getOptions(); | ||
const normalizationDepth = (options && options.normalizeDepth) || 3; // default state normalization depth to 3 | ||
|
||
const newStateContext = { state: { type: 'pinia', value: transformedState } }; |
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.
H: Currently, the state context needs to be nested twice with state
(docs here). This could change in the future as it is a bit confusing, but right now it still has to look that way to be understood by Sentry.
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.
This is assigned under store
key in the context, so I guess it should comply with the current interface.
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.
Ah right, I see it's there: scope.setContext('state', newStateContext);
packages/vue/src/pinia.ts
Outdated
filename: 'pinia_state.json', | ||
data: JSON.stringify(store.$state), |
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.
L: Can there be multiple files (because the linked event has two)? Users could benefit from having a timestamp in the filename as there can be multiple files in the attachments. Something like hh:mm_pinia_state
would be enough.
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.
Looks like these two are two different stores. So, I updated the file name format to include both the store id and the timestamp. Sample Event
packages/vue/src/pinia.ts
Outdated
actionTransformer: (action: any) => any; | ||
stateTransformer: (state: any) => any; |
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.
I think it would be nice to have tests for those :)
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.
Added 👍 c7cfa72
...(hint.attachments || []), | ||
{ | ||
filename, | ||
data: JSON.stringify(store.$state), |
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.
M: What just came to my mind: the store can contain PII, especially when pinia is used in forms. What do you think of adding the type of the value as the value? Like this:
name: string
likes: object
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.
Since the pinia plugin needs to be added manually I would argue it is fine. Just having the type is basically useless for debugging.
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.
Stringifying the state as JSON will only work in some scenarios and fail in many others like Dates, Errors, Maps, Sets, and custom classes. I would recommend using https://github.com/Rich-Harris/devalue like Nuxt does to allow users to provide serializer and revivers. I would even recommend adding a custom one that uses a toJSON()
method if it exists by default as a convention. I think, but I'm not sure, that Nuxt has something similar.
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.
Hi, we are on a tight bundle-size budget so we need to be a bit careful what we pull in as deps.
What we definitely should do is wrap the stringify call in a try-catch because it can throw in certain situations.
Non-serializable data is tricky and I would recommend we do it in a follow-up if at all. Dates get serialized properly with ordinary JSON.stringify, errors, maps, and sets, will become objects (which is fine for now), and stringify already supports toJSON natively.
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.
The toJSON()
was specific to devalue ofc. If users can opt-in to a fully compatible serializable solution, they would be able to put their app in a similar state with the serialized state and debug in dev mode, so I think it's nice to advocate for that! Since Map and Sets are common, it would be a shame to lose them fully by using JSON.stringify()`
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.
So, this event processor logic is already in try-catch, so we're safe on that part. We can do a follow-up for serializing. Thanks for the review @posva!
packages/vue/src/pinia.ts
Outdated
type SentryPiniaPluginOptions = { | ||
attachPiniaState?: boolean; | ||
actionTransformer: (action: any) => any; | ||
stateTransformer: (state: any) => any; |
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.
In addition to having the entire options object be optional, I think all of the options individually should probably also be optional.
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.
Updated: c8482b9
...(hint.attachments || []), | ||
{ | ||
filename, | ||
data: JSON.stringify(store.$state), |
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.
Since the pinia plugin needs to be added manually I would argue it is fine. Just having the type is basically useless for debugging.
const transformedAction = options.actionTransformer(context.name); | ||
|
||
if (typeof transformedAction !== 'undefined' && transformedAction !== null) { | ||
addBreadcrumb({ |
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.
I would add an option to turn off creating breadcrumbs. From experience, some apps have a lot of state updates and breadcrumbs might be very spammy.
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.
Updated: ea7ba22
packages/vue/src/pinia.ts
Outdated
|
||
scope.setContext('state', newStateContext); | ||
} else { | ||
scope.setContext('state', null); |
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.
Is there a way for us not to nuke the entire state context here but only the pinia state 🤔 I know it could be tricky and is a bit edge-casy but may be worth it.
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.
Does this update make sense? 4a78c04
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.
yes! perfect
cd409dc
to
4a78c04
Compare
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 🙌
Resolves: #13279 Depends on: #13840 [Sample Event](https://sentry-sdks.sentry.io/issues/5939879614/?project=5429219&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&sort=date&statsPeriod=1h&stream_index=0) Docs PR: getsentry/sentry-docs#11516 Adds a Pinia plugin with a feature set similar to the Redux integration. - Attaches Pinia state as an attachment to the event (`true` by default) - Provides `actionTransformer` and `stateTransformer` to the user for potentially required PII modifications. - Adds breadcrumbs for Pinia actions - Assigns Pinia state to event contexts.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.34.0` -> `8.35.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.34.0/8.35.0) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.34.0` -> `8.35.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.34.0/8.35.0) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@​sentry/node)</summary> ### [`v8.35.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8350) [Compare Source](getsentry/sentry-javascript@8.34.0...8.35.0) ##### Beta release of the official Nuxt Sentry SDK This release marks the beta release of the `@sentry/nuxt` Sentry SDK. For details on how to use it, check out the [Sentry Nuxt SDK README](https://github.com/getsentry/sentry-javascript/tree/develop/packages/nuxt). Please reach out on [GitHub](https://github.com/getsentry/sentry-javascript/issues/new/choose) if you have any feedback or concerns. - **feat(nuxt): Make dynamic import() wrapping default ([#​13958](getsentry/sentry-javascript#13958 (BREAKING) - **feat(nuxt): Add Rollup plugin to wrap server entry with `import()` ([#​13945](getsentry/sentry-javascript#13945 **It is no longer required to add a Node `--import` flag. Please update your start command to avoid initializing Sentry twice (BREAKING CHANGE).** The SDK will now apply modifications during the build of your application to allow for patching of libraries during runtime. If run into issues with this change, you can disable this behavior in your `nuxt.config.ts` and use the `--import` flag instead: ```js sentry: { dynamicImportForServerEntry: false; } ``` - **feat(nuxt): Respect user-provided source map generation settings ([#​14020](getsentry/sentry-javascript#14020 We now require you to explicitly enable sourcemaps for the clientside so that Sentry can un-minify your errors. We made this change so source maps aren't accidentally leaked to the public. Enable source maps on the client as follows: ```js export default defineNuxtConfig({ sourcemap: { client: true, }, }); ``` - feat(nuxt): Log server instrumentation might not work in dev ([#​14021](getsentry/sentry-javascript#14021)) - feat(nuxt): Add Http `responseHook` with `waitUntil` ([#​13986](getsentry/sentry-javascript#13986)) ##### Important Changes - **feat(vue): Add Pinia plugin ([#​13841](getsentry/sentry-javascript#13841 Support for [Pinia](https://pinia.vuejs.org/) is added in this release for `@sentry/vue`. To capture Pinia state data, add `createSentryPiniaPlugin()` to your Pinia store: ```javascript import { createPinia } from 'pinia'; import { createSentryPiniaPlugin } from '@​sentry/vue'; const pinia = createPinia(); pinia.use(createSentryPiniaPlugin()); ``` - **feat(node): Implement Sentry-specific http instrumentation ([#​13763](getsentry/sentry-javascript#13763 This change introduces a new `SentryHttpInstrumentation` to handle non-span related HTTP instrumentation, allowing it to run side-by-side with OTel's `HttpInstrumentation`. This improves support for custom OTel setups and avoids conflicts with Sentry's instrumentation. Additionally, the `spans: false` option is reintroduced for `httpIntegration` to disable span emission while still allowing custom `HttpInstrumentation` instances (`httpIntegration({ spans: false })`). - **feat(core): Make stream instrumentation opt-in ([#​13951](getsentry/sentry-javascript#13951 This change adds a new option `trackFetchStreamPerformance` to the browser tracing integration. Only when set to `true`, Sentry will instrument streams via fetch. ##### Other Changes - feat(node): Expose `suppressTracing` API ([#​13875](getsentry/sentry-javascript#13875)) - feat(replay): Do not log "timeout while trying to read resp body" as exception ([#​13965](getsentry/sentry-javascript#13965)) - chore(node): Bump `@opentelemetry/instrumentation-express` to `0.43.0` ([#​13948](getsentry/sentry-javascript#13948)) - chore(node): Bump `@opentelemetry/instrumentation-fastify` to `0.40.0` ([#​13983](getsentry/sentry-javascript#13983)) - fix: Ensure type for `init` is correct in meta frameworks ([#​13938](getsentry/sentry-javascript#13938)) - fix(core): `.set` the `sentry-trace` header instead of `.append`ing in fetch instrumentation ([#​13907](getsentry/sentry-javascript#13907)) - fix(module): keep version for node ESM package ([#​13922](getsentry/sentry-javascript#13922)) - fix(node): Ensure `ignoreOutgoingRequests` of `httpIntegration` applies to breadcrumbs ([#​13970](getsentry/sentry-javascript#13970)) - fix(replay): Fix onError sampling when loading an expired buffered session ([#​13962](getsentry/sentry-javascript#13962)) - fix(replay): Ignore older performance entries when starting manually ([#​13969](getsentry/sentry-javascript#13969)) - perf(node): Truncate breadcrumb messages created by console integration ([#​14006](getsentry/sentry-javascript#14006)) Work in this release was contributed by [@​ZakrepaShe](https://github.com/ZakrepaShe) and [@​zhiyan114](https://github.com/zhiyan114). Thank you for your contributions! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xMjguNiIsInVwZGF0ZWRJblZlciI6IjM4LjEyOC42IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=--> Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/251 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>
Resolves: #13279
Depends on: #13840
Sample Event
Docs PR: getsentry/sentry-docs#11516
Adds a Pinia plugin with a feature set similar to the Redux integration.
true
by default)actionTransformer
andstateTransformer
to the user for potentially required PII modifications.