-
-
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
reactComponentAnnotation creates Bad Requests (400) when fetching from external data source such as Algolia #12720
reactComponentAnnotation creates Bad Requests (400) when fetching from external data source such as Algolia #12720
Comments
@0Calories Would you mind taking a look at this if you got the time? Thanks! |
Tagging @Haroenv - this looks like an issue on our end (prop bleeding?), thought I would expect the Algolia SDK to guard from this. |
@JonasBa what do you mean by our end in this scenario? Sentry SDK, Algolia SDK? |
@smeubank I think we generate these annotations, but then they somehow end up in Algolia, which I would expect the Algolia SDK to guard from (unless we are doing something really bad here) |
The JS SDK will take any parameter and pass it on to the network. I guess what may happen in this case is you pass an extra prop to Configure, which then gets turned into a search parameter. As we can't know in advance what all possible parameters are, we don't have an allowlist. Maybe sentry needs some configuration to prevent attaching these props to certain components? |
@Haroenv Please see the following not as passing around a hot potato but rather from a purely technical standpoint 😂: I think it would be more reasonable if the Algolia SDK had a denylist. Our plugin runs during build and in some situations, it is semantically impossible (without doing a completely unreasonable amount of computations) to know what component we are attaching attributes to. cc @0Calories because he owns the annotation plugin. |
It's possible to solve the problem in InstantSearch indeed (workaround / patch can be here: algolia/instantsearch#6380) but I'm not convinced that every component can take any arbitrary property. Surely there are more components that will break if you pass the component annotations? |
@Haroenv You're probably right. The reality is that the vast majority of components in the ecosystem can take arbitrary properties. It is rather uncommon for components to do something with every prop passed. Especially if the props are prefixed with We're looking into ways of making the plugin more robust - I have a hunch that it may be really hard. Of course not putting blame on anybody here. |
I'm working on getsentry/sentry-javascript-bundler-plugins#617 which will add a configuration to specify components that should not have annotations applied. Users have brought up incompatibilities with other libraries so I do think it is important to have this option available on Sentry's side as well |
We could even have a list of components as a default, that we know are potentially problematic. |
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/react
SDK Version
8.11.0
Framework Version
18.3.1
Link to Sentry event
No response
SDK Setup/Reproduction Example
Steps to Reproduce
Here is my webpack config:
Expected Result
When reactComponentAnnotation is enabled I should not have a response from algolia indicating a failed request of 400.
Actual Result
The text was updated successfully, but these errors were encountered: