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

fix(node): Make sure modulesIntegration does not crash esm apps #14169

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Nov 4, 2024

resolves #12500
resolves #14165

modulesIntegration uses top-level require which will crash ESM apps if you explicitly import and use the integration.

// index.mjs
Sentry.init({
    dsn: '__DSN__',
    integrations: [
        Sentry.modulesIntegration(),
    ]
});

This fixes that by adding a boolean check for cjs apps, and logging out a warning as a result.

@AbhiPrasad AbhiPrasad requested review from timfish and a team November 4, 2024 14:38
@AbhiPrasad AbhiPrasad self-assigned this Nov 4, 2024
@AbhiPrasad AbhiPrasad requested review from lforst and andreiborza and removed request for a team November 4, 2024 14:38
Copy link
Contributor

github-actions bot commented Nov 4, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.76 KB - -
@sentry/browser - with treeshaking flags 21.54 KB - -
@sentry/browser (incl. Tracing) 35.15 KB - -
@sentry/browser (incl. Tracing, Replay) 71.87 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.3 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 76.19 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.99 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.83 KB - -
@sentry/browser (incl. metrics) 27 KB - -
@sentry/browser (incl. Feedback) 39.9 KB - -
@sentry/browser (incl. sendFeedback) 27.4 KB - -
@sentry/browser (incl. FeedbackAsync) 32.2 KB - -
@sentry/react 25.51 KB - -
@sentry/react (incl. Tracing) 38.12 KB - -
@sentry/vue 26.9 KB - -
@sentry/vue (incl. Tracing) 37.04 KB - -
@sentry/svelte 22.9 KB - -
CDN Bundle 24.1 KB - -
CDN Bundle (incl. Tracing) 36.97 KB - -
CDN Bundle (incl. Tracing, Replay) 71.63 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.97 KB - -
CDN Bundle - uncompressed 70.66 KB - -
CDN Bundle (incl. Tracing) - uncompressed 109.73 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 222.25 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 235.46 KB - -
@sentry/nextjs (client) 38.2 KB - -
@sentry/sveltekit (client) 35.75 KB - -
@sentry/node 129.68 KB +0.05% +60 B 🔺
@sentry/node - without tracing 94.4 KB +0.09% +85 B 🔺
@sentry/aws-serverless 105.24 KB +0.06% +60 B 🔺

View base workflow run

@timfish
Copy link
Collaborator

timfish commented Nov 4, 2024

I don't think we can rely on checking at integration creation time and I think much of our current usage of isCjs is slightly floored. If you start Sentry via --require it will detect CJS but then the error could later occur in ESM code where require is not available. require(esm) will make this more likely.

I've seen similar in nextJs where Sentry detects CJS but nextjs is just loading instrument.js and Sentry in CJS mode. Underneath nextjs runs in ESM and uses async import to load dependencies including any cache handler which hits import-in-the-middle.

I think we need to remove isCjs entirely and gate at usages of require, module.register or anything else that is CJS only.

@timfish
Copy link
Collaborator

timfish commented Nov 4, 2024

maybe we need this integration to be built to .cjs so we know it's always run in cjs mode 🤔

Copy link
Collaborator

@timfish timfish left a comment

Choose a reason for hiding this comment

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

This is fine to merge. I will open another issue for my concerns once they're properly formed and tested 😂

@AbhiPrasad
Copy link
Member Author

I think we need to remove isCjs entirely and gate at usages of require, module.register or anything else that is CJS only.

Yes isCjs needs to be fixed, but I figured this at least gets around the app crashing issue. We can keep iterating from here.

@AbhiPrasad AbhiPrasad force-pushed the abhi-esm-modules-integration branch from ce068e5 to e21fc2d Compare November 4, 2024 17:06
@AbhiPrasad AbhiPrasad merged commit 3f953f9 into develop Nov 4, 2024
129 checks passed
@AbhiPrasad AbhiPrasad deleted the abhi-esm-modules-integration branch November 4, 2024 18:14
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.

Infinite error loop with modulesIntegration in ESM modulesIntegation throws error if used from ESM
3 participants