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

ref(node): Allow node stack parser to work in browser context #5135

Merged
merged 3 commits into from
May 27, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented May 19, 2022

Depending on configuration, Electron may have mixed Chrome/node.js stack traces and to decode these we need to use the node stack parser in the Electron renderers. Because the default Electron configuration does not have mixed Chrome/node.js context, for maximum compatibility, our renderer code must always be browser/bundler compatible which poses some issues when using the node.js stack parser.

The only node.js specific code in the stack parser fetches the module using node.js features (require.main.filename, global.process.cwd()). This PR makes the getModule function an optional parameter and reorganises the file structure so we can import the required code without hitting anything node specific which will upset bundlers:

import { nodeStackLineParser } from '@sentry/node/stack-parser';

@@ -44,9 +44,8 @@ export {

export { NodeClient } from './client';
export { makeNodeTransport } from './transports';
export { defaultIntegrations, init, lastEventId, flush, close, getSentryRelease } from './sdk';
export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk';
Copy link
Member

Choose a reason for hiding this comment

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

Why not export nodeStackLineParser at the top level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we export from the top level and then import from there, will we not hit issues with bundlers trying to target the browser?

I just have painful memories of bundlers pulling in everything and erroring before they've even had a chance to do the treeshaking magic and remove the non-compatible code.

Copy link
Member

Choose a reason for hiding this comment

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

This is a fair point - maybe we just stick with this for now then.

@AbhiPrasad
Copy link
Member

You might have to rebase because some lint failures sneaked into 7.x, but we fixed them.

@lforst lforst merged commit b0919ef into getsentry:7.x May 27, 2022
@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone May 30, 2022
@timfish timfish deleted the feat/node-stack-parse-browser branch August 31, 2022 16:03
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.

3 participants