sentry-javascript-bundler-plugins icon indicating copy to clipboard operation
sentry-javascript-bundler-plugins copied to clipboard

reactComponentAnnotation. I use an unintended value for a prop.

Open jin-Pro opened this issue 1 year ago • 8 comments

Problem Statement

Hello, I am a software developer using @sentry/vite-plugin.

I am using reactComponentAnnotation in sentryVitePlugin.

I was thinking of injecting the corresponding value into html's attribute through babel.

However, I saw data-sentry-component and data-sentry-source-file in react component prop, Because of this value I faced error.

I think this function interferes with the spread operation, which is a basic function of javascript. Because of this, I argue that this feature should be changed.

---------------- Additional explanation ---------------------

I used sentry library in my project.

The packaged version of the library used in my project.

"vite": "^5.4.10", "@sentry/react": "^8.35.0", "@sentry/vite-plugin": "^2.22.6", Sentry's reactComponentAnnotation feature injects properties like data-sentry-component and data-sentry-source-file into component props.

This can cause unintended errors when using methods like Object.keys() to process props, as unexpected keys are included.

For example, in logic where components are dynamically rendered based on specific keys, these additional properties can lead to unintended behavior.

const testObjValue = {
  a: ComponentA,
  b: ComponentB,
};

type ComponentType = {
  a: any;
  b: any;
};

const Component = (props: ComponentType) => {
  const keys = Object.keys(props) as ['a', 'b'];
  return (
    <ul>
      {keys.map((key) => {
        const ChildComponent = testObjValue[key];
        return <ChildComponent key={key} />; 
      })}
    </ul>
  );
};

Solution Brainstorm

The reactComponentAnnotation feature of Sentry should be modified to directly add attributes only to the HTML tags in JSX.
This approach avoids modifying the component's props and affects only the HTML DOM, effectively preventing unintended side effects.

jin-Pro avatar Feb 26 '24 07:02 jin-Pro

Hi @jin-Pro, thanks for writing in!

If you could provide a minimal reproducible example for the issue we'd greatly appreciate it! Also please feel free to propose a concrete solution.

Gonna tag @0Calories to take a look please, thx!

Lms24 avatar Feb 26 '24 09:02 Lms24

I think this function interferes with the spread operation, which is a basic function of javascript.

Can confirm that is an issue using the webpack plugin as well. Specifically, we were unable to spread Sets. 🤷‍♂

Resolution is to not use reactComponentAnnotation in the mean time.

mattleong avatar Aug 16 '24 01:08 mattleong

@mattleong could you specify what you are trying to do exactly, or better, provide a small reproducible example?

chargome avatar Aug 16 '24 07:08 chargome

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

getsantry[bot] avatar Nov 23 '24 08:11 getsantry[bot]

I used sentry library in my project.

The packaged version of the library used in my project.

  • "vite": "^5.4.10",
  • "@sentry/react": "^8.35.0",
  • "@sentry/vite-plugin": "^2.22.6",

Sentry's reactComponentAnnotation feature injects properties like data-sentry-component and data-sentry-source-file into component props.

This can cause unintended errors when using methods like Object.keys() to process props, as unexpected keys are included.

For example, in logic where components are dynamically rendered based on specific keys, these additional properties can lead to unintended behavior.

const testObjValue = {
  a: ComponentA,
  b: ComponentB,
};

type ComponentType = {
  a: any;
  b: any;
};

const Component = (props: ComponentType) => {
  const keys = Object.keys(props) as ['a', 'b'];
  return (
    <ul>
      {keys.map((key) => {
        const ChildComponent = testObjValue[key];
        return <ChildComponent key={key} />; 
      })}
    </ul>
  );
};

The reactComponentAnnotation feature of Sentry should be modified to directly add attributes only to the HTML tags in JSX.
This approach avoids modifying the component's props and affects only the HTML DOM, effectively preventing unintended side effects.

@Lms24 @0Calories

jin-Pro avatar Nov 27 '24 00:11 jin-Pro

I have @sentry/nextjs 9.1.0 and I tried to use the new ignoredComponents to fix the @rect-three/fiber problem like so:

ignoredComponents : ['@react-three/fiber', '__r3f', 'r3f', 'PlaneGeometry', 'BufferGeometry', 'Canvas', 'ambientLight', 'DirectionalLight', 'AdaptiveDpr', 'EffectComposer', 'Bloom', 'OrthographicCamera', 'PerspectiveCamera', 'SoftShadows', 'mesh']

I tried the package name "@react-three/fiber", some name the package uses internally like "r3f", the name of the component that I use in react like "Canvas", other components from react-three-fiber like "DirectionalLight" and none worked, I still have the same conflict, where react three fiber throws an error as it does not recognize the attributes

One problem is that the term "ignoredComponents" is ambigous, I had a quick look at the code an saw that actually two things get ussed to check against the ignore list, one is components names and another one seems to be element names, in the end it is confusing as I have no clue if I need to add a displayname to a component for it to actually have a name, and for third party packages like react-three-fiber which are not just one component but a multitude of nested components makes me wonder if I need to exclude each of them

After reading @jin-Pro message, I think this is maybe the reason why my attempt to exclude the right component failed

Also something else I wondered, if I ignore a component I assume that it will only ignore the component itself and not its children?

In my case if I could ignore my own component that uses react three fiber and it would mean that all nested components get ignored too, that would probably solve my problem

do we have a repro? do you need one?

EDIT: ping https://github.com/getsentry/sentry-javascript-bundler-plugins/issues/530#event-16167779539

chrisweb avatar Feb 21 '25 19:02 chrisweb

Let me suggest another example of using tanstackQuery.

// in ComponentA/index.tsx

type ComponentAProps = {
  page:number;
  size:number;
}

const ComponentA = (props:ComponentAProps) => {
  const datas = useQuery({
    queryKey: ['post', props],
    queryFn: () => getPosts(props),
  })
  ...
}


// in ComponentB/index.tsx

type ComponentBProps = {
  page:number;
  size:number;
}

const ComponentB = (props:ComponentBProps) => {
  const datas = useQuery({
    queryKey: ['post', props],
    queryFn: () => getPosts(props),
  })
  ...
}

// in another Component

const Component = () => {
  return <>
    <ComponentA page={1} size={10} /> // queryKey : ['posts' , { page : 1, size: 10, data-sentry-component: 'ComponentA', data-sentry-source-file:'ComponentA/index.tsx' }
    <ComponentB page={1} size={10} />  // queryKey : ['posts' , { page : 1, size: 10, data-sentry-component: 'ComponentB', data-sentry-source-file:'ComponentB/index.tsx' }
  </>
}

In this way, adding specific fields to React Component Props can cause errors and unintended behavior for users.

Thank you.

jin-Pro avatar Feb 22 '25 12:02 jin-Pro

One problem is that the term "ignoredComponents" is ambigous, I had a quick look at the code an saw that actually two things get ussed to check against the ignore list, one is components names and another one seems to be element names, in the end it is confusing as I have no clue if I need to add a displayname to a component for it to actually have a name, and for third party packages like react-three-fiber which are not just one component but a multitude of nested components makes me wonder if I need to exclude each of them

@chrisweb if you have nested components you want to ignore you would indeed need to add them separately as of now

@0Calories Could you have a look at this one please?

chargome avatar Feb 24 '25 08:02 chargome