-
Notifications
You must be signed in to change notification settings - Fork 16
Fix decorators overrides #658
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
Conversation
✔️ All good! |
383758a
to
5e83537
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.
Please have a look at the broken build on CI and don't forget to add a changeset.
I'm also okay with making a breaking change as long as the version bump matches.
return this | ||
} | ||
|
||
public getAction(): string | undefined { |
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.
Can you add some unit tests for these new helpers?
@tombruijn I took a look at the mismatches between types, and got so frustrated at the structure of the packages that I folded the In my view, this change removes a significant source of typing, packaging and distribution headaches, and was worth doing at some point. Let me know if you disagree, and also let me know if you think it deserves a greater version bump, or if marking |
@unflxw I'm all for simplifying this integration. If we have less packages, go for it. From what I understand other packages do rely on these core and types packages, but also needed the appsignal/javascript package to function anyway. Let me know when the build is green and I can review it :) |
25362ac
to
12c147a
Compare
12c147a
to
cc4b6d8
Compare
The return value from decorators and overrides is ignored. Instead of the returned span, the initial span (which may have been modified in-place by the decorators or override) is used. Delete the `compose` function, which is now unused.
Add getter methods for the span's properties, so users can inspect the state of the span in overrides and make decisions based on it, without having to dig into the undocumented `._data` property.
Run the logic to ignore an error based on its message and the `ignoreErrors` configuration option after overrides have ran. This allows overrides to be used to tweak the message to trigger or skip the behaviour of this configuration option.
cc4b6d8
to
727b482
Compare
Allow for a span to be ignored by an override returning `false` from it. Along with the getters, this allows customers to implement their own span ignoring logic.
Remove the `core` and `types` packages, folding their contents into the main `javascript` package. This causes a dependency on the `javascript` package where there previously was one on `types`, but this applies to packages that have no meaningful usage patterns independently from the `javascript` package.
727b482
to
f9dd80c
Compare
Use return value from hooks
The return value from decorators and overrides is ignored. Instead
of the returned span, the initial span (which may have been modified
in-place by the decorators or override) is used.
Delete the
compose
function, which is now unused.Add span getters
Add getter methods for the span's properties, so users can inspect
the state of the span in overrides and make decisions based on it,
without having to dig into the undocumented
._data
property.Ignore errors after overrides
Run the logic to ignore an error based on its message and the
ignoreErrors
configuration option after overrides have ran. Thisallows overrides to be used to tweak the message to trigger or skip
the behaviour of this configuration option.
Allow ignoring spans in overrides
Allow for a span to be ignored by an override returning
false
fromit. Along with the getters, this allows customers to implement their
own span ignoring logic.