"captureRemixServerException" is not exported by "node_modules/@sentry/remix/esm/index.client.js"

[gustavopch]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.80.1

Framework Version

2.3.0

Link to Sentry event

No response

SDK Setup

Check the link to StackBlitz below.

Steps to Reproduce

1. Open https://stackblitz.com/edit/remix-run-remix-ikymah?file=app%2Futils.ts.
2. Run npx vite build to build the client bundle.

Expected Result

The file node_modules/@sentry/remix/esm/index.client.js should probably export a dummy captureRemixServerException with an implementation like new Error("Not implemented for client").

Check this comment by Hiroshi on Remix's repo for more context: remix-run/remix#7973 (comment)

Actual Result

[Lms24]

Hi @gustavopch thanks for writing in and thanks for investigating!

If you follow the docs to set up Sentry in Remix (e.g. via the wizard), does this warning also occur? Furthermore, just to understand the impact - does this warning degrade functionality or is it just a warning?

I'm not sure why our entry points (module vs browser) don't seem to work in this case for Vite<>Remix. For instance, we use the exact same mechanism to distinguish between client and server builds in our SvelteKit SDK where we also don't export dummy functions in the opposite index file.
I've read the linked comment and the explanations make sense to me but I still don't understand why this would only be problematic in Remix. At least I haven't heard of this being a problem in other full stack framework SDKs of ours.

Anyway, if this is how we can fix it we'll probably do it. Just wanna gather some more information before we apply this fix.

[hi-ogawa]

@Lms24 Thanks for triaging the issue!
I don't have an exact use case of @gustavopch so it might not be correct, but answering based on my understanding:

does this warning degrade functionality or is it just a warning?

As I wrote in the original comment, I think this is only a warning and the app should be functional as long as it doesn't actually call Sentry.captureRemixServerException on client code (on browser).

we use the exact same mechanism to distinguish between client and server builds in our SvelteKit SDK where we also don't export dummy functions in the opposite index file.

Sorry for extremely artificial repro..., but I think technically the same warning can be observed on @sentry/sveltekit integration as well. I made a reproduction based on their stackblitz template:

https://stackblitz.com/edit/sveltejs-kit-template-default-d7udwt?file=src%2Fhooks.client.ts

reveal screenshot

I think the condition to get this warning is to include the reference to "server only export names" in client build, which I don't think usually happens if user's server/client code is structured well.
So, personally, I don't think this is something sentry side needs to be sensitive about.
(But it would be still helpful to have an issue here for visibility for others, so thanks gustavopch for submitting an issue!)

[Lms24]

@hi-ogawa thanks a lot for jumping in! I see, this makes sense to me now.

I think the condition to get this warning is to include the reference to "server only export names" in client build, which I don't think usually happen if user's server/client code is structured well.
So, personally, I don't think this is something sentry side needs to be sensitive about.

I tend to agree and I'd like to avoid adding dummy functions if reasonably possible. I get that it's sometimes hard to distinguish if functions belong to the server or client part of a package but generally, users shouldn't call server code in client parts (and vice versa). @gustavopch any chance that this is the reason for the warning on your end? I'd like to understand your use case better to determine if there's action needed on our end.

[gustavopch]

@Lms24 I have app/monitoring.ts that exports functions encapsulating all code that handles logging and error tracking, and it's meant to be isomorphic. In particular, Log.e(message, data, options) logs to the console, sends to Logtail and also to Sentry. The options parameter tells if it should be sent to Sentry as a Remix error boundary error, a Remix server exception or a regular exception. To illustrate, here's how it will be used in entry.server.ts:

export const handleError = (error: unknown, { request }: DataFunctionArgs) => {
  if (!request.signal.aborted) {
    Log.e('💥 Error caught by handleError:', error, {
      sentryMethod: 'captureRemixServerException',
      request,
    })
  }
}

So captureRemixServerException() won't ever be called on the client, but it's imported regardless because app/monitoring.ts is meant to be isomorphic. I tried to wrap with if (import.meta.env.SSR) as it seemed to be the ideal solution, but the warning was still there. As a workaround, I'm using a dynamic import just for that function so Vite doesn't consider it in its static analysis, but I think there must be a better way.

[hi-ogawa]

Thanks for providing more contexts.
I also thought if (import.meta.env.SSR) would make it dead-code, but it might be that rollup analyze imports/exports before considering dead code.

Btw, I just came across the idea of silencing certain rollup warning in a different context TanStack/query#5161 (comment) and I think this can be applied to this scenario as well:

https://stackblitz.com/edit/remix-run-remix-2tt6qz?file=vite.config.ts

import { unstable_vitePlugin as remix } from '@remix-run/dev';
import { defineConfig } from 'vite';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
  plugins: [remix(), tsconfigPaths()],
  build: {
    rollupOptions: {
      onwarn(message, defaultHandler) {
        if (
          message.code === 'MISSING_EXPORT' &&
          message.binding === 'captureRemixServerException'
        ) {
          // we can silence this warning by just return
          console.log('++++++++++++');
          console.log(message);
          console.log('++++++++++++');
          return;
        }
        defaultHandler(message);
      },
    },
  },
});

screenshot

I'm still unsure whether sentry side should have consistent exports or user code should workaround it only when necessary.

[Lms24]

Hey, thanks for the great discussion. I can see both points. We'll discuss internally to decide how we go about this.

[gustavopch]

Workaround for Remix + Vite using https://github.com/pcattori/vite-env-only:

serverOnly$(() => {
  Sentry.captureRemixServerException(data, 'remix.server', options.request)
})!()

The Vite plugin will remove the code inside serverOnly$() from the client bundle.

[wouterds]

It's been a while since the last update, what's the status?

[Lms24]

Hi @wouterds, no update so far on this. We're currently in the final stretch of releasing our new major version of the SDKs and all our time is spent there.

My take (note, no final decision here): I'm still not convinced that we should add this due to the cost of increasing client bundle size but if folks really prefer the size hit over ensuring that there code runs where it should run, we can add it. Just need some time to get it in. PRs are welcome :)

[AbhiPrasad]

@onurtemizkan the changes from #12110 should address this right?

[onurtemizkan]

@AbhiPrasad, no this is not addressed there, but I can open a PR if there is a consensus about exporting it from the client-side.

[AbhiPrasad]

Yes let's add a dummy implementation, with a debug log warning!

[AbhiPrasad]

Fix released with https://github.com/getsentry/sentry-javascript/releases/tag/8.10.0!