asyncapi / generator

Use your AsyncAPI definition to generate literally anything. Markdown documentation, Node.js code, HTML documentation, anything!
https://asyncapi.com/docs/tools/generator
Apache License 2.0
755 stars 211 forks source link

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

Closed bakasmarius closed 1 week ago

bakasmarius commented 9 months ago

Describe the bug

I originally stumbled upon this bug when I started using nestjs-asyncapi package (https://github.com/flamewow/nestjs-asyncapi/issues/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 (https://github.com/asyncapi/generator/issues/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.

Rakeshkraki commented 6 months ago

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.

github-actions[bot] commented 2 months ago

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

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 :heart:

bakasmarius commented 2 months ago

Could anyone please look into this?

derberg commented 2 weeks ago

isn't it fixed with https://github.com/asyncapi/generator/pull/1165 ?

bakasmarius commented 2 weeks ago

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).

derberg commented 2 weeks ago

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

could you open a PR that removes not needed code?