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.

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

feat(remix): Add Remix 2.x release support. #8940

Merged
merged 15 commits into from
Sep 19, 2023
Merged

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Sep 4, 2023

Resolves: #8681
closes #9040

Summary:

  • Updated Node.JS version used on E2E tests to 18.x. (This is required for Remix v2.x, and 18.x is the current LTS so updated this for all) chore(e2e-tests): Use Node 18 for E2E tests. #8964
  • Updated create-remix-app E2E tests to cover client-side events and transactions.
  • Added create-remix-app-v2 E2E test application that uses 2.0.0 version.

Events:

  • Client-side captured event - Link
  • Client-side render error - Link
  • Server-side error - Link
  • pageload transaction - Link
  • navigation transaction - Link

@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 3 times, most recently from 6efcf2c to 0149461 Compare September 4, 2023 15:00
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.58 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.49 KB (0%)
@sentry/browser - Webpack (gzipped) 22.09 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 70.27 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.59 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.66 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 222.15 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.64 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.49 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.47 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.61 KB (0%)
@sentry/react - Webpack (gzipped) 22.12 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.49 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 51.07 KB (0%)

@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 2 times, most recently from abb124e to b6ec1a3 Compare September 5, 2023 10:31
@onurtemizkan onurtemizkan changed the title feat(remix): Add flags for Remix v2 usage. feat(remix): Add flags for Remix 2.x usage. Sep 5, 2023
@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 3 times, most recently from aef6350 to ff4d574 Compare September 6, 2023 10:39
@onurtemizkan onurtemizkan marked this pull request as ready for review September 6, 2023 11:31
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we open up a separate PR for the node version changes on the e2e test? That way this PR can only look at changes to remix.

@@ -818,7 +819,7 @@ jobs:
- name: Set up Node
uses: actions/setup-node@v3
with:
node-version-file: 'package.json'
node-version: 18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we read the e2e-tests package.json here instead?

@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 3 times, most recently from 9b935d0 to 67c3c4e Compare September 7, 2023 12:46
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing before we merge - is there no way to get the remix version automatically? I would prefer not forcing folks to use the isRemixV2 flag.

@onurtemizkan
Copy link
Collaborator Author

onurtemizkan commented Sep 8, 2023

One last thing before we merge - is there no way to get the remix version automatically? I would prefer not forcing folks to use the isRemixV2 flag.

Yes, I thought about it, there's no property including version available on client side (window.__remixContext), it's possible on server-side though. Is there a reliable way to pass that info on build time from server to client SDK?

Update: Passed remix version from server package.json to browser env while monkey-patching root loader.

@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 4 times, most recently from e62e166 to 09a1425 Compare September 11, 2023 18:54
@onurtemizkan onurtemizkan marked this pull request as draft September 11, 2023 18:55
@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch 7 times, most recently from 5eb3369 to 2ab1c11 Compare September 12, 2023 19:18
@huw
Copy link

huw commented Sep 16, 2023

Just FYI after upgrading my own setup:

  • error instanceof Error doesn’t work anymore for distinguishing between true client-side and ‘hydrated’ errors; we have to check for the presence of error.stack
  • Remix exports ErrorResponse from all runtimes and isRouteErrorResponse from @remix-run/react works fine for detection in all runtimes
  • ErrorResponses in handleError should probably therefore be sent to captureRemixServerException in the recommended setup (Remix now correctly emits them for Remix-internal errors that don’t end up in the error boundary; we just have to test for status >= 500)
  • captureRemixServerException should check for ErrorResponse instead of Response; I don’t think there are any situations where it would be sent a Response in Remix v2 but am much less sure of this than the others.
  • ErrorResponses that are sent to captureRemixServerException often include errorResponse.error, which is a private field in TypeScript but can be unwrapped to get an ordinary error to send to Sentry. This isn’t true for loader/action ErrorResponses.

Comment on lines 50 to 51
const pkg = loadModule<{ version: string }>('@remix-run/react/package.json');
const version = pkg ? pkg.version : '0.0.0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: If we stick with this approach (see my other comment), let's add a debug log if we fail to load package.json

@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch from 081e762 to 88a7f23 Compare September 18, 2023 11:04
@sergical
Copy link

Remix V2 is out, would love to get this in 😅

@HazAT HazAT mentioned this pull request Sep 18, 2023
@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch from 5917bf9 to d6badeb Compare September 19, 2023 09:39
@onurtemizkan
Copy link
Collaborator Author

Thanks for the trial @huw, it's been very helpful!

error instanceof Error doesn’t work anymore for distinguishing between true client-side and ‘hydrated’ errors; we have to check for the presence of error.stack

I was not able to reproduce this, but will take another look after the release.

ErrorResponses that are sent to captureRemixServerException often include errorResponse.error, which is a private field in TypeScript but can be unwrapped to get an ordinary error to send to Sentry. This isn’t true for loader/action ErrorResponses.

This may need a bit of discussion and new test cases.

We can revisit these two points as improvements in other PRs. The other 3 points @huw reported are now included in the first version.

@onurtemizkan onurtemizkan requested a review from Lms24 September 19, 2023 10:03
@onurtemizkan onurtemizkan force-pushed the onur/remix-v2-e2e-tests branch from d6badeb to 7164807 Compare September 19, 2023 10:05
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now! I agree with merging this in now and tackling the open questions in follow up tasks/PRs. Thanks Onur!

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ship this as a first step - thanks Onur!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to remix v2 Make sure Remix SDK works without feature flags
5 participants