feat(remix): Add Fastify server adapter #11261

rustworthy · 2024-03-23T17:37:44Z

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

mydea · 2024-03-25T09:15:31Z

Hello, thank you for this PR!

Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!

rustworthy · 2024-03-25T11:48:27Z

Hello, thank you for this PR!

Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!

My pleasure! I am now setting up things for e2e manual test. As for the wrapper itself, what I have learned so far from the codebase, we need to know ahead of time how to parse out details from the request data. We also need to know the name of the response's end method, to flush the data to remote repo prior to resolving

onurtemizkan

Thanks for this @rustworthy. Looks good to me generally. Could you rebase and add e2e tests please?

onurtemizkan · 2024-04-05T10:10:42Z

Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!

@mydea, yes I was also curious about it, had some trials to remove adapters, but looks like we need to keep those for the time being.

rustworthy · 2024-04-06T15:00:05Z

packages/remix/test/integration/package.json

    "@mcansh/remix-fastify": "^3.2.2",
    "@remix-run/express": "1.17.0",
    "@remix-run/node": "1.17.0",
    "@remix-run/react": "1.17.0",
    "@remix-run/serve": "1.17.0",
    "@sentry/remix": "file:../..",
+    "express": "^4.19.2",


Suggesting to have express explicitly listed as a dependency in this package. The fact that the upper level dev package is powered by express may not be true in the future. Plus it makes it clear, adapters for which frameworks are being covered here.

That makes sense to me 👍

rustworthy · 2024-04-06T16:31:42Z

packages/remix/package.json

-    "@types/express": "^4.17.14"
+    "@types/express": "^4.17.14",
+    "fastify": "^4.26.2",
+    "typescript": "^5.4.4"


Hopefully we can use latest tsc for the package. Otherwise, it's typing the exported wrapper for fastify is problematic, since the compiler is not sure that the handler is acually a RouteMethodHandler which it is.

We can loosely type our exported wrapper instead of bumping the TS version. We apply TS version updates to all packages together when we do.

Ok, I did not know there was a policy or convention with regard to tsc used. As for loosely typing, that will mean they will need to assert like as RouteMethodHandler that somehow seems not that ergonomic, but if it's a common (or at leas acceptable) practice, let's put it this way. Btw, the only reason the fastify package has been added - is to type the handler, we are otherwise good, since the internals of the request have already been described in the polimorphic request types definitions.

rustworthy · 2024-04-06T16:45:30Z

packages/remix/test/integration/test/server/utils/helpers.ts

-    const requestHandlerFactory =
-      adapter === 'express' ? wrapExpressCreateRequestHandler(createRequestHandler) : createRequestHandler;
+const adapters = {
+  [Adapter.Builtin]: createExpressRequestHandler,


built-in is actually a createRequestHandler from @remix-run/express. Does this mean it actually does not need a wrapper?

Yes, we're auto-instrumenting the built-in createRequestHandler from @remix-run/node. So this doesn't need a wrapper for built-in tests.

onurtemizkan

Just a few comments about the TS version and testing, but overall looks good to me.

onurtemizkan · 2024-04-08T21:46:12Z

packages/remix/tsconfig.json

@@ -5,6 +5,7 @@

  "compilerOptions": {
    "jsx": "react",
-    "module": "es2020"
+    "module": "es2020",
+    "ignoreDeprecations": "5.0"


We won't need this unless we update the TS version right?

that's right, and that commit has been reverted

onurtemizkan · 2024-04-08T21:55:08Z

packages/remix/test/integration/package.json

@@ -7,11 +7,15 @@
    "start": "remix-serve build"
  },
  "dependencies": {
+    "@fastify/formbody": "^7.4.0",
+    "@mcansh/remix-fastify": "^3.2.2",


This is breaking the tests on Node 14 / 16.
I'd suggest adding an e2e test application under dev-packages/e2e-tests/test-applications for Fastify where we can control the environment better and also can test more extensively, instead of adding it to the integration test matrix.

Well, I've added an app with remix-express-vite template with the idea to update it to use fastify engine and add e2e tests. This is embarrassing but I cannot make the app I've added resolve the local @sentry/remix correctly, and so cannot navigate around the test app routes, neither build it. Moreover, I am having hard times with running the test app that's already there - the create-remix-app-express-vite-dev: there's a proxy there, some customization in .npmrc and also pnpm manager mentions in the package.json . You can imagine, my first try was to cp -r the mentioned app and update where necessary, but I just failed to install the deps 🥲

Can you please assist in setting up the test app, but only in the scope of making the local copy of @sentry/remix (where the fastify wrapper is already included in the build) resolve correctly? I would have looked up the solution from another test app but seems like all them are using the latest npm copy of the lib. I'll then add the fastify framework and e2e tests myself.

If you pull the branch, and run npm run dev from inside the newly added dev-packages/e2e-tests/test-applications/create-remix-app-fastify-vite app, and visit the index page, you will see the error and maybe immediately know the answer.

Again, I am super embarrassed I need to ask for assistance here.

@rustworthy, not at all! Been there 😅

There may be a couple of reasons for this local resolution issue, but the steps I use are as below:

yarn build:tarball the whole monorepo.

pnpm store prune to make sure the latest tarball is published to the test registry.

yarn test:e2e or yarn test:run <test-app-name> to run the tests.

I'll pull the branch locally and test, do you mind if I push commits to this branch if needed, to save you time?

Thanks a lot for sticking with this! ❤️

Please feel free to push onto this branch directly and thanks for giving a hand here!

@rustworthy, I have set up the e2e test application for you, there seem to be failing cases, you can run tests as I mentioned above.

@onurtemizkan Thanks again for your help! I should have paid more attention to the right tools management (volta) and the proxy-registry trick. Added a trouble-shooting section to e2e-tests/README.md (maybe for myself in the future 😅 ).

As for the test app, I am now on it and planning to do the following:

add missing standard front-end tests and also back-end tests and make sure they are passing with express and the existing wrapExpressCreateRequestHandler;

switch to fastify with the corresponding wrapFastifyCreateRequestHandler and make sure tests are still passing.

just an update: in progress

@onurtemizkan Please have a look

I see the tsc is not happy in remix integration tests - I'll fix this.

@onurtemizkan Done.

This reverts commit ebcf709.

rustworthy · 2024-06-01T08:40:35Z

What's the destiny of this PR guys?

cc @onurtemizkan

mydea · 2024-06-03T09:22:28Z

I would think that with v8, where we have native support for fastify, this should not be necessary anymore...? Did you have the chance to try it out, so using @sentry/remix v8 out of the box, with no special fastify code?

rustworthy · 2024-06-03T10:40:50Z

I would think that with v8, where we have native support for fastify, this should not be necessary anymore...? Did you have the chance to try it out, so using @sentry/remix v8 out of the box, with no special fastify code?

I think I got a déjà vu and it's super confusing 😕

I remember we agreed these changes were necessary, and we got them implemented and tested. If that's not needed in sentry@v8 why would we still have express specific test apps in the dev-packages? Should they be swapped for remix and remix-vite?

I am super ok if these changes are not needed, but can the maintainers then close this PR with a corresponding comment, so that we gracefully finish our work? Leaving it behind this way with no feedback is not super motivating for people to contribute to the project 😉

mydea · 2024-06-04T08:37:16Z

yeah sorry about this, I get that this is confusing, I am a bit confused myself 😅

@onurtemizkan recently opened a PR to use a new remix instrumentation: #12110

@onurtemizkan could you chime in if that PR would supersede/"fix" what this PR here has set out to do? Or if that will still be relevant?

onurtemizkan · 2024-06-04T10:58:59Z

Sorry for the late response @rustworthy and @mydea. I've been trying to recover from jet lag 😅.

Ideally, #12110 should make Fastify covered out of the box, like Express. That PR is the result of an experiment that turned out to work fine to replace the Express server adapter. So I would expect it to work for Fastify as well. Though, I did not have the bandwidth to test Fastify on that branch. We still may need a part of the updates in this PR (at least the new tests for sure), so I'd suggest keeping this PR open until we finalise #12110 and validate the Fastify usage if that makes sense to you guys. If everything goes well on that side, we can merge in the tests here.

rustworthy mentioned this pull request Mar 24, 2024

Makes sense to substitute Express for Fastify? epicweb-dev/epic-stack#670

Closed

onurtemizkan reviewed Apr 5, 2024

View reviewed changes

rustworthy force-pushed the feat/remix-fastify-adapter branch 2 times, most recently from 03f49b6 to 1806704 Compare April 6, 2024 06:01

rustworthy commented Apr 6, 2024

View reviewed changes

rustworthy requested a review from onurtemizkan April 6, 2024 16:45

onurtemizkan changed the title ~~Remix fastify server adapter~~ feat(remix): Add Fastify server adapter Apr 8, 2024

onurtemizkan requested changes Apr 8, 2024

View reviewed changes

rustworthy force-pushed the feat/remix-fastify-adapter branch from ab6b9e3 to 5245b4f Compare April 10, 2024 06:54

rustworthy requested a review from onurtemizkan April 16, 2024 18:39

rustworthy added 15 commits April 18, 2024 13:54

Add fastify types to polymorphics

e3397e0

Add fastify branches to requestdata parsing

63ddafb

Re-use logic for Express and Fastify adapters

7b2b971

Ingore .yarn dir

4231514

Turn 'SupportedFramework' to string enum

578f1da

Check for '__sentry_fastify_wrapped__' prop in 'instrumentServer' mod

0a5dfa8

chore: add remix-fastify adapter to remix/test package

3231279

chore: untrack .yarn dir

f5f1671

chore: rm generated d.ts for gatsby

72309fe

chore: add 'Fastify' to integrations tests

d294533

chore: use form body parser in fastify app

01b3d4b

chore: use fastify types

ec96d48

Revert "chore: use fastify types"

ea61973

This reverts commit ebcf709.

Rm fastify-remix from integrtions tests

18e567f

chore: add remix-fastify-vite test app

a495c7f

onurtemizkan force-pushed the feat/remix-fastify-adapter branch from 99133e5 to 059008b Compare April 19, 2024 09:57

Setup E2E test application.

ee6cb16

onurtemizkan force-pushed the feat/remix-fastify-adapter branch from 059008b to ee6cb16 Compare April 19, 2024 09:59

rustworthy added 16 commits April 20, 2024 13:21

Upd e2e-tests/README with troubleshooting section

0cbb034

Add tests for client-side integration

1f8f58f

Fix 'wrapRequestHandler' args order

920680d

Update type hits to honor tsc

15a7a0c

Clean up, add color-preferece honoring bg

3b37721

Add authenticated axious client as test helper

78e4d22

Add tests pass with released @sentry/remix and fastify engine

f8bcdb2

Pipe playwrigth logs to stdout

fb15ad8

Merge branch 'develop' into feat/remix-fastify-adapter

838de36

Merge branch 'develop' into feat/remix-fastify-adapter

60ba722

Use inspect and .txt. ext for Sentry server events

849e6fd

Return reply for Fastify adapter

a7bcba8

Update GenericRequiestHandler signature

3f0b5ea

Set transaction name in wrapRequestHandler

cb6d651

Apply biome fmt fixes

aeb5cb2

Merge branch 'develop' into feat/remix-fastify-adapter

373dffd

onurtemizkan self-assigned this Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remix): Add Fastify server adapter #11261

feat(remix): Add Fastify server adapter #11261

rustworthy commented Mar 23, 2024 •

edited

Loading

mydea commented Mar 25, 2024

rustworthy commented Mar 25, 2024

onurtemizkan left a comment

onurtemizkan commented Apr 5, 2024

rustworthy Apr 6, 2024

onurtemizkan Apr 8, 2024

rustworthy Apr 6, 2024

onurtemizkan Apr 8, 2024

rustworthy Apr 10, 2024

rustworthy Apr 6, 2024

onurtemizkan Apr 8, 2024

onurtemizkan left a comment

onurtemizkan Apr 8, 2024

rustworthy Apr 10, 2024

onurtemizkan Apr 8, 2024

rustworthy Apr 16, 2024

onurtemizkan Apr 17, 2024

rustworthy Apr 17, 2024

onurtemizkan Apr 19, 2024

rustworthy Apr 20, 2024

rustworthy Apr 26, 2024

rustworthy Apr 29, 2024

rustworthy Apr 29, 2024

rustworthy Apr 29, 2024

rustworthy commented Jun 1, 2024

mydea commented Jun 3, 2024

rustworthy commented Jun 3, 2024

mydea commented Jun 4, 2024

onurtemizkan commented Jun 4, 2024

feat(remix): Add Fastify server adapter #11261

Are you sure you want to change the base?

feat(remix): Add Fastify server adapter #11261

Conversation

rustworthy commented Mar 23, 2024 • edited Loading

mydea commented Mar 25, 2024

rustworthy commented Mar 25, 2024

onurtemizkan left a comment

Choose a reason for hiding this comment

onurtemizkan commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurtemizkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustworthy commented Jun 1, 2024

mydea commented Jun 3, 2024

rustworthy commented Jun 3, 2024

mydea commented Jun 4, 2024

onurtemizkan commented Jun 4, 2024

rustworthy commented Mar 23, 2024 •

edited

Loading