Skip to content
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(react-component-annotate): Allow skipping annotations on specified components #617

Merged
merged 13 commits into from
Jan 8, 2025

Conversation

0Calories
Copy link
Member

@0Calories 0Calories commented Oct 8, 2024

Note: The large number of deleted lines is due to a lot of old tests no longer being relevant since the component ignore config system is being overhauled

Some third party libraries involve components that have compatibility issues with the react-component-annotate plugin, and results in errors when the data-sentry properties are added as props. This PR introduces a bit of a bandaid fix by introducing an option to allow users to configure the names of components that should not have any annotations applied.

When using the standalone Babel plugin, you can configure this option like so:

// babel.config.js

{
  // ... other config above ...

  plugins: [
    // Put this plugin before any other plugins you have that transform JSX code
    // The options are set by providing an object as the second element in the array, but not required
    ['@sentry/babel-plugin-component-annotate', {ignoreComponents: ['Foo', 'Bar']}]
  ],
}

When using the Sentry bundler plugins, you can configure this option like so:

sentryVitePlugin({
    // ... other options
    reactComponentAnnotation: {enabled: true, ignoreComponents: ['Foo', 'Bar']},
}),

TODO in future: Allow providing a source file location, since it is possible for multiple components to have the same name, but you may not want to skip annotation on all of these.

@0Calories 0Calories self-assigned this Oct 8, 2024
@lforst
Copy link
Member

lforst commented Dec 19, 2024

@0Calories can we expedite this PR?

@0Calories
Copy link
Member Author

@lforst I'll try to finish it on friday, I believe it just needs tests

Also:

allow providing a source file location, since it is possible for multiple components to have the same name, but you may not want to skip annotation on all of these.

This unfortunately will have to be done in a followup PR. For now, I want to at least expose the option to skip specific components by names as that will at least provide an option for users who are struggling with incompatible components

@nathan-gage
Copy link

nathan-gage commented Dec 20, 2024

will it be possible to resolve #611 with this PR -- i.e., not trying to name <React.Fragment/>s used in headless?

@0Calories
Copy link
Member Author

Hi @nathan-gage, unfortunately I don't think this would be able to help with that Headless UI issue. It's possible that you could include React.Fragment as a component name to be skipped, but I can't confirm whether that would work or not

@0Calories 0Calories marked this pull request as ready for review January 7, 2025 01:31
@0Calories
Copy link
Member Author

@lforst This is good to go for a review now!

@lforst lforst self-assigned this Jan 7, 2025
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lgtm, I think I would call it ignoredComponents though. Maybe a test or two would also be dope.

Comment on lines 326 to 330
// TODO: Support this option in the future
/**
* A list of strings representing local paths to files to ignore. The plugin will not apply `data-sentry` annotations on components in these files
*/
// ignoreFiles?: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove this comment and create an issue instead.

Comment on lines 400 to 407
// TODO: Support this option in the future
// {
// name: "ignoreFiles",
// type: "string[]",
// fullDescription:
// "A list of strings representing local paths to files to ignore. The plugin will not perform any annotations on components in these files.",
// supportedBundlers: ["webpack", "vite", "rollup"],
// },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@@ -47,6 +47,14 @@ Using pnpm:
pnpm add @sentry/babel-plugin-component-annotate --save-dev
```

## Options

### `ignoreComponents`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit but I think my logic is sound: I think I would probably call it ignore*d*Components. ignoreComponents sounds like imperative and that it would accept a boolean rather than a list of components.

@lforst lforst removed their assignment Jan 8, 2025
@0Calories
Copy link
Member Author

Thanks for the review @lforst! Agreed on the naming of ignoredComponents. There's also a test I added but it's buried in the diffs because of all the obsolete tests I deleted

@0Calories 0Calories merged commit e1975d4 into main Jan 8, 2025
18 checks passed
@0Calories 0Calories deleted the ash/feat/annotate-plugin-ignore-options branch January 8, 2025 16:22
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Jan 11, 2025
| datasource | package             | from   | to     |
| ---------- | ------------------- | ------ | ------ |
| npm        | @sentry/vite-plugin | 2.22.7 | 2.23.0 |


## [v2.23.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#2230)

-   chore(deps): bump nanoid from 3.3.6 to 3.3.8 ([#641](getsentry/sentry-javascript-bundler-plugins#641))
-   feat(core): Detect Railway release name ([#639](getsentry/sentry-javascript-bundler-plugins#639))
-   feat(core): Write module injections to `globalThis` ([#636](getsentry/sentry-javascript-bundler-plugins#636))
-   feat(react-component-annotate): Allow skipping annotations on specified components ([#617](getsentry/sentry-javascript-bundler-plugins#617))
-   ref(core): Rename release management plugin name ([#647](getsentry/sentry-javascript-bundler-plugins#647))

Work in this release contributed by [@conor-ob](https://github.com/conor-ob). Thank you for your contribution!
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Jan 11, 2025
| datasource | package             | from   | to     |
| ---------- | ------------------- | ------ | ------ |
| npm        | @sentry/vite-plugin | 2.22.7 | 2.23.0 |


## [v2.23.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#2230)

-   chore(deps): bump nanoid from 3.3.6 to 3.3.8 ([#641](getsentry/sentry-javascript-bundler-plugins#641))
-   feat(core): Detect Railway release name ([#639](getsentry/sentry-javascript-bundler-plugins#639))
-   feat(core): Write module injections to `globalThis` ([#636](getsentry/sentry-javascript-bundler-plugins#636))
-   feat(react-component-annotate): Allow skipping annotations on specified components ([#617](getsentry/sentry-javascript-bundler-plugins#617))
-   ref(core): Rename release management plugin name ([#647](getsentry/sentry-javascript-bundler-plugins#647))

Work in this release contributed by [@conor-ob](https://github.com/conor-ob). Thank you for your contribution!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants