feat(core): Automatically annotate React components at build time #464
Conversation
| @@ -52,6 +52,8 @@ | |||
| "fix": "eslint ./src ./test --format stylish --fix" | |||
| }, | |||
| "dependencies": { | |||
| "@babel/core": "7.18.5", | |||
There was a problem hiding this comment.
Had to move this to a regular dependency, since it is necessary to run transformAsync from @babel/core during the transform hook
- placeholder tests - simple contrived "app" - necessary test files
That way we don't have to including it in the package just for testing.
Instead of checking bundle output, run the output through a real React renderer, and check its output against a snapshot. This makes more a more honest integration test.
- add necessary Rollup build dependencies - add spec and build pipeline
Compilation errors only show up in `stats`!
- add minimal viable Webpack 5 config - add snapshot test
Version 9 is not compatible with Webpack 4
…s://github.com/getsentry/sentry-javascript-bundler-plugins into experiment/react-annotate-plugin-esbuild-tests
0Calories
changed the title
|
In the process of fixing some lint errors and some more cleanup, but the bulk of this PR is done now. Opening it for review |
Log error separately, since its type is not known
There was a problem hiding this comment.
Okay initial review - but this PR needs to be split up to make it easier to review/understand.
- A PR that just vendors in
packages/bundler-plugin-core/src/plugins/react-annotate-plugin.js, but makes it TS. Ideally we port any unit tests, or write ones as appropriate. - A PR that adds
react-annotate-pluginto core package, adds it to one of webpack/esbuild/vite/rollup, and adds e2e test accordingly - A PR that adds plugin to all the rest of the plugins, and updates docs in the package (grep for
OPTIONS_SECTION_INSERTandgenerateOptionsDocumentation()in the repo for docs info).
| const idWithoutQueryAndHash = stripQueryAndHashFromPath(id); | ||
|
|
||
| if (idWithoutQueryAndHash.match(/\\node_modules\\|\/node_modules\//)) { | ||
| return null; |
There was a problem hiding this comment.
m: is returning null safe here? Do we have to return { code } instead?
There was a problem hiding this comment.
Good point, null is accepted by Unplugin as one of the return values of the transform hook, and I assumed it means that the file skips any transformations. However, I can't see any specific documentation on what returning null does in this hook
There was a problem hiding this comment.
However, we already return null in our release injection plugin, under the transform hook for these same situations. Example: https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/main/packages/bundler-plugin-core/src/index.ts#L388
| @@ -510,6 +532,59 @@ export function createRollupDebugIdUploadHooks( | |||
| }; | |||
| } | |||
|
|
|||
| export function createReactAnnotateHooks() { | |||
| return { | |||
| async transform(code: string, id: string): Promise<TransformResult> { | |||
There was a problem hiding this comment.
m: can we add caching to this transform?
| // id may contain query and hash which will trip up our file extension logic below | ||
| const idWithoutQueryAndHash = stripQueryAndHashFromPath(id); | ||
|
|
||
| if (idWithoutQueryAndHash.match(/\\node_modules\\|\/node_modules\//)) { |
There was a problem hiding this comment.
m: Does this work with windows file paths?
There was a problem hiding this comment.
This code is copied from another transform hook in our release injection plugin, so I assume so (but if not, it's broken in that plugin too :p)
| } | ||
|
|
||
| if (name.indexOf("/") !== -1) { | ||
| return name.split("/").pop(); |
There was a problem hiding this comment.
l: IMO we should lift up magic constants into it's own variables.
| * Options related to react component name annotations. | ||
| * Disabled by default, unless a value is set for this option | ||
| */ | ||
| reactAnnotate?: { |
There was a problem hiding this comment.
l: Let's make this option name a bit more detailed
| reactAnnotate?: { | |
| reactComponentNameAnnotate?: { |
Handle strings, nulls, and objects.
That function should not reference `this` since that function is used without binding.
- less redundant snapshots - avoids false positives for obsolete snapshots
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
…/getsentry/sentry-javascript-bundler-plugins into experiment/react-annotate-plugin
| function esbuildReactAnnotatePlugin(): UnpluginOptions { | ||
| const pluginName = "sentry-esbuild-react-annotate-plugin"; | ||
| return { | ||
| name: pluginName, | ||
| esbuild: { | ||
| setup({ onLoad }) { | ||
| onLoad({ filter: /\.(t|j)sx$/ }, async (args) => { | ||
| const code = await fsPromises.readFile(args.path, "utf8"); | ||
| const loader = args.path.endsWith(".tsx") ? "tsx" : "jsx"; | ||
| const results = await createReactAnnotateHooks().transform(code, args.path); | ||
|
|
||
| let contents: string = code; | ||
|
|
||
| if (typeof results === "string") { | ||
| contents = results; | ||
| } else if (results?.code) { | ||
| contents = results.code; | ||
| } | ||
|
|
||
| return { | ||
| loader, | ||
| pluginName, | ||
| contents, | ||
| }; | ||
| }); | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Unfortunately, we cannot use a simple onLoad hook for this funcitonality esbuild plugin, and in fact, I believe we cannot do this with good conscience in esbuild at all. Esbuild doesn't have a transform hook and that is by design. Read this comment I wrote for a similar situation: #428 (comment)
I don't feel like pressing the "changes requested" button but please consider this as a pretty hard blocker!
There was a problem hiding this comment.
Hey Luca, thanks for letting me know. I see, that's quite unfortunate that we can't use onLoad like this :/
Would you know of any alternatives that we can use to support this functionality in esbuild?
I assume the biggest problem here is that the onLoad hook only loads in files once, so this will prevent other loaders from running on these same files. In that case, can we fix this by using a namespace?
The esbuild docs says this:
A callback added using onLoad will be run for each unique path/namespace pair that has not been marked as external.
It sounds like as long as we assign a unique namespace during the onResolve hook, we can get around this. Please lmk if this is an acceptable solution :)
There was a problem hiding this comment.
The plugin should not add much computational overhead as well, since it is an O(n) time algorithm. All it's doing is parsing JSX files and adding additional properties onto JSX elements
There was a problem hiding this comment.
I honestly do not think you can do this in esbuild with good conscience. If you assign a different namespace to the files in onResolve you will prevent the file from being picked up by other plugins' onLoad hooks and breaking them. This might be fine in a lot of cases but as soon as people add their own onLoad hooks this will break. I'll leave it up to you but just so you're aware.
There was a problem hiding this comment.
Understood, in that case we will hold off on releasing this plugin to esbuild for now, but we can reconsider it in the future if customers ask for it 👍
|
Closing this PR, I'm currently in the process of breaking it down into incremental PRs |
Adds an opt-in feature to the bundler plugins that will automatically add annotations to React components in JSX and TSX files, and their resulting HTML DOM nodes. This is done via an underlying Babel plugin that is run during build time. To enable the feature, set
reactAnnotate.enabledtotrue.Example of an automatically annotated component in Sentry's frontend:
This will allow the browser SDK to pick off component names and send them to Sentry, allowing for some very useful features:
The groundwork of the above features are already in place, now we just need to rollout this plugin to customers and observe! Adding it in the bundler plugins allows for an easy way to gain access to this feature, without requiring our customers to muck about in the build configs and manually add the Babel plugin.
This PR comes with integration tests, and has also been manually tested on all supported bundlers: Vite, ESBuild, Rollup, Webpack
Due to the experimental nature of this feature, it's opt in for now, but we eventually will want it enabled by default so as many customers as possible can get access to the capabilities that it unlocks. Of course, loader logic will need to be added where applicable so that performance is not impacted