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): Refactor node stack parsing to use common parser #4612

Merged
merged 65 commits into from
Feb 24, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Feb 21, 2022

This PR:

  • Converts the node stack parser to be line orientated and return StackFrame | undefined
  • Uses the common parser from @sentry/utils
  • Updates the tests to cater for this
    • StackFrame[] are reversed
    • Additional fields added
  • I have kept the top SyncPromise in the backend and removed promises from where they are not necessary.

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.

2 quick comments

packages/node/src/stack-parser.ts Outdated Show resolved Hide resolved
packages/node/src/stack-parser.ts Outdated Show resolved Hide resolved
}

const base = `${
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd()
Copy link
Member

Choose a reason for hiding this comment

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

We can use optional chaining in node land since we down-compile when we transpile from TS -> JS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to revert the optional chaining because it broke a webpack test somewhere.

I'm going to have to some back the require issue later!

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, wonder what's going on. Is it the tests we have in https://github.com/getsentry/sentry-javascript/tree/master/packages/node/test/manual?

I think this is partly why we have utils like

export function dynamicRequire(mod: any, request: string): any {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return mod.require(request);
}

Copy link
Collaborator Author

@timfish timfish Feb 24, 2022

Choose a reason for hiding this comment

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

It was actually the nextjs webpack tests that started failing.

The issue with dynamicRequire is that it's only designed to work with module.require.

module.require only gives you access to the require function. It does not include any other properties like require.main. This is only available on require which isn't even a regular global.

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, TIL about module.require details.

@AbhiPrasad AbhiPrasad added this to the Pre 7.0.0 Work milestone Feb 23, 2022
@timfish timfish marked this pull request as ready for review February 24, 2022 12:24
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
packages/node/src/stack-parser.ts Outdated Show resolved Hide resolved
}

const base = `${
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd()
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, wonder what's going on. Is it the tests we have in https://github.com/getsentry/sentry-javascript/tree/master/packages/node/test/manual?

I think this is partly why we have utils like

export function dynamicRequire(mod: any, request: string): any {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return mod.require(request);
}

packages/node/src/stack-parser.ts Outdated Show resolved Hide resolved
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.

Think we are good to merge after the node parser name change, and fixing the rebase in the issue template!

}

const base = `${
(require && require.main && require.main.filename && dirname(require.main.filename)) || global.process.cwd()
Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, TIL about module.require details.

packages/node/test/stacktrace.test.ts Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) February 24, 2022 19:25
@AbhiPrasad AbhiPrasad merged commit bb6f865 into getsentry:master Feb 24, 2022
@timfish timfish deleted the node/refactor-stack-parsing branch February 25, 2022 01:18
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.

None yet

3 participants