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

Installing 'source-map-support' package results in wrong call stack #1067

Closed
bakasmarius opened this issue Nov 17, 2023 · 6 comments · Fixed by #1258
Closed

Installing 'source-map-support' package results in wrong call stack #1067

bakasmarius opened this issue Nov 17, 2023 · 6 comments · Fixed by #1258
Labels
bug Something isn't working

Comments

@bakasmarius
Copy link
Contributor

Describe the bug

I originally stumbled upon this bug when I started using nestjs-asyncapi package (flamewow/nestjs-asyncapi#516).
Importing one of their decorators resulted in wrong call stack for failing tests.
I narrowed it down a bit to a place where nestjs-asyncapi imports import Generator from '@asyncapi/generator' - commenting out that line resulted in correct call stacks.
Then I dug deeper and found the actual cause: https://github.com/asyncapi/generator/blob/master/lib/utils.js#L155 - commenting out just that one line resulted in correct call stacks.
Apparently, https://github.com/evanw/node-source-map-support#installation-and-usage is not needed anymore for Node >=12.12.0.
My suggestion is to remove source-map-support package from @asyncapi/generator, but I don't know whether that would break anything.

P.S.: this is a second bug (#1030) which is caused by executing code outside the constructor. From the user/developer perspective, merely importing a function or a decorator (or anything else for that matter) from a package should not DO anything until you call or apply what you imported.

How to Reproduce

Run a simple test (I use jest):

it('test', () => {
  expect(1).toEqual(2)
})

the resulting call stack is correct:

expect(received).toEqual(expected) // deep equality

    Expected: 2
    Received: 1

      1 | it('test', () => {
    > 2 |   expect(1).toEqual(2)
        |             ^
      3 | })
      4 |

      at Object.<anonymous> (test.spec.ts:2:13)

Now run the same test with require('source-map-support').install() line added:

it('test', () => {
  require('source-map-support').install()
  expect(1).toEqual(2)
})

the resulting call stack is incorrect:

    expect(received).toEqual(expected) // deep equality

    Expected: 2
    Received: 1

      2 |   require('source-map-support').install()
      3 |   expect(1).toEqual(2)
    > 4 | })
        |   ^
      5 |

      at Object.<anonymous> (test.spec.ts:4:15)

Expected behavior

Importing @asyncapi/generator (or any other library that imports it) should not impact the call stack of failing tests.

@bakasmarius bakasmarius added the bug Something isn't working label Nov 17, 2023
@Rakeshkraki
Copy link

Steps to Reproduce:

Run a test (using Jest or similar test framework) without importing source-map-support.

it('test', () => {
expect(1).toEqual(2);
});
Notice the resulting correct call stack.

Run the same test with the require('source-map-support').install() line added before the test.

it('test', () => {
require('source-map-support').install();
expect(1).toEqual(2);
});
Observe the resulting incorrect call stack.

Expected Behavior:

Importing @asyncapi/generator or any package that imports it should not impact the call stack of failing tests. The call stack should remain correct regardless of the imported packages.

Suggested Solution:

Remove the source-map-support package from @asyncapi/generator, as it is no longer needed for Node versions greater than or equal to 12.12.0. This change should resolve the issue without causing any further disruptions, but it is recommended to thoroughly test the functionality after making this change to ensure compatibility and stability.

Copy link
Contributor

github-actions bot commented Jul 8, 2024

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jul 8, 2024
@bakasmarius
Copy link
Contributor Author

Could anyone please look into this?

@github-actions github-actions bot removed the stale label Jul 9, 2024
@derberg
Copy link
Member

derberg commented Aug 20, 2024

isn't it fixed with #1165 ?

@bakasmarius
Copy link
Contributor Author

bakasmarius commented Aug 21, 2024

isn't it fixed with #1165 ?

I don't think so, because #1165 only removed registerTypeScript(), but registerSourceMap() is still present.
I just tested it with vitest (we don't use jest anymore) and I can confirm that as long as require('source-map-support').install() is called, this bug persists.

As I mentioned before, source-map-support is not needed for Node >=12.12.0 (https://github.com/evanw/node-source-map-support?tab=readme-ov-file#node-12120) and looks like you support >=18.12.1 (https://github.com/asyncapi/generator/blob/master/package.json#L28).

Copy link
Member

derberg commented Aug 21, 2024

yup, with latest major version we dropped node 12 support.

could you open a PR that removes not needed code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants