Skip to content

ref(node): Refactor node stack parsing to use common parser - #4612

Merged
AbhiPrasad merged 65 commits into
getsentry:masterfrom
timfish:node/refactor-stack-parsing
Feb 24, 2022
Merged

ref(node): Refactor node stack parsing to use common parser#4612
AbhiPrasad merged 65 commits into
getsentry:masterfrom
timfish:node/refactor-stack-parsing

Conversation

@timfish

@timfishtimfish commented Feb 21, 2022

Copy link
Copy Markdown
Collaborator

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.

timfish added 30 commits June 20, 2021 22:20

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 quick comments

Comment threadpackages/node/src/stack-parser.ts Outdated
Comment threadpackages/node/src/stack-parser.ts Outdated
Comment threadpackages/node/src/stack-parser.ts Outdated
}

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
Contributor

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

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

@timfishtimfishFeb 24, 2022

Copy link
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
Contributor

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.

@timfish
timfishforce-pushed the node/refactor-stack-parsing branch from 68c47fc to b7966ceCompareFebruary 23, 2022 21:38
@AbhiPrasadAbhiPrasad 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
Comment thread.github/ISSUE_TEMPLATE/bug.yml Outdated
Comment threadpackages/node/src/stack-parser.ts Outdated
}

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

Copy link
Copy Markdown
Contributor

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

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

Comment threadpackages/node/src/stack-parser.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

Comment threadpackages/node/test/stacktrace.test.ts
@AbhiPrasad
AbhiPrasad enabled auto-merge (squash) February 24, 2022 19:25
@timfish
timfishforce-pushed the node/refactor-stack-parsing branch from 5ec09d7 to 1379939CompareFebruary 24, 2022 20:24
@AbhiPrasad
AbhiPrasad merged commit bb6f865 into getsentry:masterFeb 24, 2022
@timfish
timfish deleted the node/refactor-stack-parsing branch February 25, 2022 01:18
Sign up for freeto 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

@timfish@AbhiPrasad@vladanpaunovic