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.

Sign up for GitHub

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

Jump to bottom

Can't instrument Remix server build with Express and Vite dev server #10724

Closed
3 tasks done
haines opened this issue Feb 19, 2024 · 3 comments · Fixed by #10784
Closed
3 tasks done

Can't instrument Remix server build with Express and Vite dev server #10724

haines opened this issue Feb 19, 2024 · 3 comments · Fixed by #10784
Assignees
@onurtemizkan
Labels
Package: remix Issues related to the Sentry Remix SDK Type: Bug

Comments

@haines
Copy link

haines commented Feb 19, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.101.1

Framework Version

2.6.0

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

Follow the Remix Vite migration guide with a custom Express app server. Specifically, use the Vite dev server in middleware mode and pass a function to createRequestHandler.

  createRequestHandler({
    build: viteDevServer
      ? () =>
          viteDevServer.ssrLoadModule(
            "virtual:remix/server-build"
          )
      : await import("./build/server/index.js"),
  })

Expected Result

Sentry instruments the server build whether it is passed as a function or a static build. If the server build is a function, Sentry wraps the function and instruments the result when it is called.

Actual Result

Sentry's monkey patch fails with an error.

TypeError: Cannot read properties of undefined (reading 'module')
  at instrumentBuild (node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected]/node_modules/@sentry/src/utils/instrumentServer.ts:519:36)
  at Object.<anonymous> (node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected]/node_modules/@sentry/src/utils/instrumentServer.ts:565:36)
  at createRequestHandler (node_modules/.pnpm/@[email protected][email protected][email protected]/node_modules/@remix-run/express/dist/server.js:36:28)

This was previously marked as resolved (#9500) by a workaround that was added to the docs where instead of passing the build as a function, you instead have to call the function and await the result. But as mentioned in this comment, that is not a good idea because it breaks HDR (the whole point of passing the function is that it is re-evaluated on every request, so that in dev you always get the latest server build after making changes, without having to reload the app server).
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 19, 2024
@github-actions github-actions bot added the Package: remix Issues related to the Sentry Remix SDK label Feb 19, 2024
@haines haines mentioned this issue Feb 19, 2024
Closed
3 tasks
@lforst
Copy link
Member

lforst commented Feb 22, 2024

Hi, sorry for the late reply, we are taking a look at this!

@onurtemizkan onurtemizkan mentioned this issue Feb 22, 2024
Merged
@kentcdodds kentcdodds mentioned this issue Feb 22, 2024
Merged
AbhiPrasad pushed a commit that referenced this issue Feb 26, 2024
@onurtemizkan
feat(remix): Add Vite dev-mode support to Express instrumentation. (#…
d570594 
…10784)

Resolves #10724 
Related: #9500

Adds dev-mode support to Remix setups with Vite and Express.

We currently accept Remix server `build` as an object to instrument. But
Remix allows `build` as a synchronous or asynchronous function that
returns the build object. Currently, it seems that functions are only
used in development servers, and not in production. So, while this
update slightly reduces `requestHandler` performance on dev servers, it
does not on production builds.

We need `build` in 2 places:

1- We instrument the loaders / actions on build, then we pass them down
to the original implementations.
2- We use the `routes` inside them to create parameterised transactions.

This update adds new internal wrappers around them to make sure that we
don't miss out on the returned / resolved values in case `build` is a
function, for both cases.

This PR also adds a new E2E test application using the latest Remix
version and Vite, and it runs the tests on `dev` mode.

We also need a documentation update to reflect this, if it gets merged.
@AbhiPrasad AbhiPrasad reopened this Feb 26, 2024
@AbhiPrasad
Copy link
Member

we merged this fix in, but now have to backport this onto the v7 branch - so re-opening issue until release is cut there.

onurtemizkan added a commit that referenced this issue Feb 26, 2024
@onurtemizkan
feat(remix): Add Vite dev-mode support to Express instrumentation. (#…
c2d21cf 
…10784)

Resolves #10724
Related: #9500

Adds dev-mode support to Remix setups with Vite and Express.

We currently accept Remix server `build` as an object to instrument. But
Remix allows `build` as a synchronous or asynchronous function that
returns the build object. Currently, it seems that functions are only
used in development servers, and not in production. So, while this
update slightly reduces `requestHandler` performance on dev servers, it
does not on production builds.

We need `build` in 2 places:

1- We instrument the loaders / actions on build, then we pass them down
to the original implementations.
2- We use the `routes` inside them to create parameterised transactions.

This update adds new internal wrappers around them to make sure that we
don't miss out on the returned / resolved values in case `build` is a
function, for both cases.

This PR also adds a new E2E test application using the latest Remix
version and Vite, and it runs the tests on `dev` mode.

We also need a documentation update to reflect this, if it gets merged.
onurtemizkan added a commit that referenced this issue Feb 26, 2024
@onurtemizkan
feat(remix): Add Vite dev-mode support to Express instrumentation. (#…
7634793 
…10784)

Resolves #10724
Related: #9500

Adds dev-mode support to Remix setups with Vite and Express.

We currently accept Remix server `build` as an object to instrument. But
Remix allows `build` as a synchronous or asynchronous function that
returns the build object. Currently, it seems that functions are only
used in development servers, and not in production. So, while this
update slightly reduces `requestHandler` performance on dev servers, it
does not on production builds.

We need `build` in 2 places:

1- We instrument the loaders / actions on build, then we pass them down
to the original implementations.
2- We use the `routes` inside them to create parameterised transactions.

This update adds new internal wrappers around them to make sure that we
don't miss out on the returned / resolved values in case `build` is a
function, for both cases.

This PR also adds a new E2E test application using the latest Remix
version and Vite, and it runs the tests on `dev` mode.

We also need a documentation update to reflect this, if it gets merged.
onurtemizkan added a commit that referenced this issue Feb 28, 2024
@onurtemizkan
feat(remix): Add Vite dev-mode support to Express instrumentation. (#…
618b7d7 
…10784)

Resolves #10724
Related: #9500

Adds dev-mode support to Remix setups with Vite and Express.

We currently accept Remix server `build` as an object to instrument. But
Remix allows `build` as a synchronous or asynchronous function that
returns the build object. Currently, it seems that functions are only
used in development servers, and not in production. So, while this
update slightly reduces `requestHandler` performance on dev servers, it
does not on production builds.

We need `build` in 2 places:

1- We instrument the loaders / actions on build, then we pass them down
to the original implementations.
2- We use the `routes` inside them to create parameterised transactions.

This update adds new internal wrappers around them to make sure that we
don't miss out on the returned / resolved values in case `build` is a
function, for both cases.

This PR also adds a new E2E test application using the latest Remix
version and Vite, and it runs the tests on `dev` mode.

We also need a documentation update to reflect this, if it gets merged.
onurtemizkan added a commit that referenced this issue Mar 7, 2024
@onurtemizkan
feat(remix): Add Vite dev-mode support to Express instrumentation. (#…
7ed95c1 
…10784)

Resolves #10724
Related: #9500

Adds dev-mode support to Remix setups with Vite and Express.

We currently accept Remix server `build` as an object to instrument. But
Remix allows `build` as a synchronous or asynchronous function that
returns the build object. Currently, it seems that functions are only
used in development servers, and not in production. So, while this
update slightly reduces `requestHandler` performance on dev servers, it
does not on production builds.

We need `build` in 2 places:

1- We instrument the loaders / actions on build, then we pass them down
to the original implementations.
2- We use the `routes` inside them to create parameterised transactions.

This update adds new internal wrappers around them to make sure that we
don't miss out on the returned / resolved values in case `build` is a
function, for both cases.

This PR also adds a new E2E test application using the latest Remix
version and Vite, and it runs the tests on `dev` mode.

We also need a documentation update to reflect this, if it gets merged.
@AbhiPrasad
Copy link
Member

Fixed released with https://github.com/getsentry/sentry-javascript/releases/tag/7.106.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@onurtemizkan onurtemizkan

Labels
Package: remix Issues related to the Sentry Remix SDK Type: Bug
Projects
GitHub Issues with 👀 2
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(remix): Add Vite dev-mode support to Express instrumentation. getsentry/sentry-javascript
4 participants
@haines @onurtemizkan @lforst @AbhiPrasad