-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 support for Hydrogen #15450
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
9559d6c
to
6b1e7b6
Compare
@@ -838,7 +838,7 @@ jobs: | |||
# See: https://github.com/actions/runner/issues/2205 | |||
if: always() && needs.job_e2e_prepare.result == 'success' && needs.job_e2e_prepare.outputs.matrix != '{"include":[]}' | |||
needs: [job_get_metadata, job_build, job_e2e_prepare] | |||
runs-on: ubuntu-20.04 | |||
runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this: cloudflare/workerd#3411
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me
const wrappedEntry = { ...build.entry, module: { ...build.entry.module } }; | ||
|
||
// Add warning? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be a permanent comment or is this left from developing?
In case it is permanent, maybe explain which warning you mean here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a leftover from development. Removed it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @onurtemizkan!
Mind opening a docs PR for this as well? We should update https://docs.sentry.io/platforms/javascript/guides/remix/frameworks/hydrogen/
This PR adds support for Shopify Hydrogen applications running on MiniOxygen runtime.
Resolves: #5610
What's updated
Issue with
node:async_hooks
(#5610 (comment))@sentry/cloudflare
has a new export path called./request
to import only the request wrapper without involvingasync.ts
which importsnode:async_hooks
. That fixes the build problems on the MiniOxygen environment.Issue with imported empty modules on MiniOxygen (Related: vitejs/vite#10612)
@sentry/remix
has a new export path called./cloudflare
to fix empty methods in the imported modules. The problem there seemed to be the re-exports that are somehow not resolved. So with this, I also updated the file structure of Remix SDK to separate the client/server/cloudflare code with their own per-folderindex
files.Issue having
loader
andaction
spanscloudflare
environments on the auto-instrumented flow. To support them, I re-introduced the span creation flow we were using before migration. That's a step back from feat(remix)!: RemoveautoInstrumentRemix
option #15074, but by default this logic is disabled. It's enabled by default wheninsturmentBuild
is imported from@sentry/remix/cloudflare
.The root server spans are not parameterised
That's because we don't have the Remix routes information on
@sentry/cloudflare
's request wrapper. I have tried updating the root span's name inside@sentry/remix/cloudflare
's logic and it works. But I have left it out for now, not to make this PR even more difficult to review.