getsentry / sentry-javascript-bundler-plugins

JavaScript Bundler Plugins for Sentry
https://sentry.io
BSD 3-Clause "New" or "Revised" License
141 stars 36 forks source link

Nextjs builds fail with Sentry and headlessui #611

Closed brandinchiu closed 2 months ago

brandinchiu commented 2 months ago

Environment

What version are you running? Etc. "@headlessui/react": "^2.1.5", "@heroicons/react": "^2.1.5", "@sentry/nextjs": "^8.28.0", "next": "14.2.8", "react": "^18", "react-dom": "^18"

I've created a point-in-time backup of my current working project here: https://github.com/brandinchiu/sentry-build-error

Disabling reactComponentAnnotation will allow the build to complete successfully.

Steps to Reproduce

  1. followed this tutorial to get Sentry installed on a blank Nextjs project: https://docs.sentry.io/platforms/javascript/guides/nextjs/
  2. create a page in Nextjs that makes use of a component from headlessui/react
  3. attempt to build the application with reactComponentAnnotation enabled.

Expected Result

Build should complete.

Actual Result

Build fails. Error is throw indicating that Nextjs prerendering failed when trying to inject Sentry annotations into components:

Error occurred prerendering page "/". Read more: https://nextjs.org/docs/messages/prerender-error

Error: Passing props on "Fragment"!

The current component <Menu /> is rendering a "Fragment".
However we need to passthrough the following props:
  - data-sentry-element
  - data-sentry-component
  - data-sentry-source-file
  - data-headlessui-state

You can apply a few solutions:
  - Add an `as="..."` prop, to ensure that we render an actual element instead of a "Fragment".
  - Render a single element as the child so that we can forward the props onto that element.
...
chargome commented 2 months ago

Hi @brandinchiu, have you tried any of the solutions of not using a fragment?

brandinchiu commented 2 months ago

It's an external UI kit, so I have limited control in this case.

Regardless, fragments are a fairly common react construct. Causing all builds to fail if one exists anywhere in the component tree is a bit extreme, imo.

chargome commented 2 months ago

@brandinchiu Yes, but the error is not originating from Sentry but from headless ui. You could just render the headless menu as a div for example: <Headless.Menu as="div" {...props} />;

This should fix your error 👍

brandinchiu commented 2 months ago

Respectfully, I disagree with the premise that this is a headlessui issue. I don't even think it's a Sentry issue.

But the Venn diagram here creates a legitimate problem, and I just can't see the solution being "never use a naked fragment as long as Sentry is installed".

Or, this needs to be clearly documented as a drawback that anyone who uses react can't use fragments without an as clause with Sentry (if this IS already documented somewhere, please share a link).

I am not working directly with headlessui. I am working with another UI kit that builds on top of headlessui.

Your suggestion above suggests that using a fragment without an as prop is wrong, and the best solution here is to just never do it.

As I mentioned in my first reply, this feels exceptionally harsh.

chargome commented 2 months ago

Well the thrown error from your reproduction repo is coming from headless ui and that is what I pointed out in my previous comment. I'm not suggesting that you should not be using any fragments, but we cannot change the fact that React fragments are not accepting props. So this is rather an implementation detail on how to pass down props to child components and this implementation detail in this case lies in headless ui – for which use case their as prop could be used.

brandinchiu commented 1 month ago

Fair enough. My solution for now will be to simply disable reactComponentAnnotation, as getting into guts of the UI kit I'm using would kind of defeat the purpose of offloading this work.

I'll spend some time with it at a later date to see if there's a way for me to backfill any missing "as" props to sort this out.

MattKevan commented 1 month ago

I've had exactly the same problem as @brandinchiu – using Catalyst, which is based on HeadlessUI, causes Sentry to error.

It might not be the fault of Headless or Catalyst, but it would be useful to have something in the documentation to say that fragments can cause problems with reactComponentAnnotation. I wasted most of the day yesterday trying to figure out the problem until I realised there wasn't much I could do as it was caused by a dependency two layers deep. I'd even started looking for alternatives to Sentry before stumbling on a Discord comment that led me to this issue.

chargome commented 1 month ago

@MattKevan sorry to hear that you spend so much time debugging this, we'll add a note on our docs!