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

Support NestJS (test with jest) #20619

Open
birkskyum opened this issue Sep 21, 2023 · 23 comments
Open

Support NestJS (test with jest) #20619

birkskyum opened this issue Sep 21, 2023 · 23 comments
Labels
bug Something isn't working node API polyfill Related to various "node:*" modules APIs node compat

Comments

@birkskyum
Copy link
Contributor

birkskyum commented Sep 21, 2023

Repro:

  • git clone https://github.com/nestjs/typescript-starter.git project (From Nest docs frontpage)
  • cd project
  • deno task test or deno task --unstable test or deno --unstable task test

Expected:

$ jest
 PASS  src/app.controller.spec.ts
  AppController
    getHello
      ✓ should return "Hello World!" (1 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.068 s
Ran all test suites.

Actual

➜ deno --unstable task test
Warning Currently only basic package.json `scripts` are supported. Programs like `rimraf` or `cross-env` will not work correctly. This will be fixed in an upcoming release.
Task test jest
Unstable API 'Deno.connect'. The --unstable flag must be provided.
@birkskyum birkskyum changed the title Can't provide flag to "Unstable API 'Deno.connect'. The --unstable flag must be provided." Support NestJS (test with jest) Sep 21, 2023
@birkskyum
Copy link
Contributor Author

birkskyum commented Sep 21, 2023

And trying to bypass this using deno --unstable run -A npm:jest yields:

➜ deno --unstable run -A npm:jest
watchman warning:  Recrawled this watch 5 times, most recently because:
MustScanSubDirs UserDroppedTo resolve, please review the information on
https://facebook.github.io/watchman/docs/troubleshooting.html#recrawl
To clear this warning, run:
`watchman watch-del '/Users/admin/repos/deno-kitchensink/nest-project' ; watchman watch-project '/Users/admin/repos/deno-kitchensink/nest-project'`

TypeError: this._child.send is not a function
    at ChildProcessWorker.initialize (file:///Users/admin/repos/deno-kitchensink/nest-project/node_modules/.deno/jest-worker@29.7.0/node_modules/jest-worker/build/workers/ChildProcessWorker.js:169:17)

@bartlomieju bartlomieju added bug Something isn't working node compat labels Sep 22, 2023
@bartlomieju
Copy link
Member

This is blocked by IPC support in node:child_process module. I will be looking into that.

@birkskyum
Copy link
Contributor Author

birkskyum commented Nov 6, 2023

With deno 1.38, I can't get past this message:

➜ deno task --unstable test           
Task test jest
Unstable API 'Deno.connect'. The --unstable flag must be provided.

@bartlomieju
Copy link
Member

@birkskyum out of curiosity - does it get any further if you create deno.json with:

{
  "unstable": ["net"]
}

?

@birkskyum
Copy link
Contributor Author

birkskyum commented Nov 6, 2023

Yes, with the deno.json I see:

➜ deno task test
Task test jest
TypeError: this._child.send is not a function
    at ChildProcessWorker.initialize (file:///Users/admin/repos/nest-test/project/node_modules/.deno/jest-worker@29.7.0/node_modules/jest-worker/build/workers/ChildProcessWorker.js:169:17)

@bartlomieju
Copy link
Member

So we're back at the missing IPC support - good news is: we figured out a way forward to provide this mechanism and I'll be working on PoC for Unix systems this week.

@malthe
Copy link

malthe commented Dec 17, 2023

You can run tests in-band using -i which doesn't require a child-process worker.

$ deno --unstable run -A npm:jest -i

But Jest relies on v8.serialize which is not implemented by Deno. The compatibility notes mention that:

[these APIs] could be polyfilled, but due inherent lack of format stability between the V8 versions, the Deno team is considering requiring a special flag to use them.

@birkskyum
Copy link
Contributor Author

with deno 1.39 i get:

➜ deno task test 
Task test jest
Error: Not implemented: v8.serialize
    at notImplemented (ext:deno_node/_utils.ts:9:9)
    at serialize (node:v8:59:3)
    at HasteMap._persist (file:///Users/admin/repos/deno-kitchensink/project/node_modules/.deno/jest-haste-map@29.7.0/node_modules/jest-haste-map/build/index.js:716:26)
    at file:///Users/admin/repos/deno-kitchensink/project/node_modules/.deno/jest-haste-map@29.7.0/node_modules/jest-haste-map/build/index.js:380:16
    at Object.runMicrotasks (ext:core/01_core.js:820:30)
    at processTicksAndRejections (ext:deno_node/_next_tick.ts:53:10)
    at runNextTicks (ext:deno_node/_next_tick.ts:71:3)
    at eventLoopTick (ext:core/01_core.js:188:21)
    at async file:///Users/admin/repos/deno-kitchensink/project/node_modules/.deno/@jest+core@29.7.0/node_modules/@jest/core/build/cli/index.js:268:9
    at async Promise.all (index 0)

@GerbenRampaart
Copy link

I tried the same thing. Deno 1.43.1.

Cloned the typescript-starter like @birkskyum

2024-05-03 17 42 33

TypeError: Class constructor Process cannot be invoked without 'new'

@GerbenRampaart
Copy link

GerbenRampaart commented May 6, 2024

Since I'm invested in seeing NestJs properly supported by Deno or Bun (we have many apis in our corporation developed in nest) I did some additional testing.

So I created a brand new project with the nest cli and made the following changes to get as close to a runnable project as possible:

  • app.controller.spec.ts was renamed to app.controller.test.ts
  • app.controller.test.ts now has a unit test according to Deno.test
  • All imports have .ts postfixed to them to prevent sloppy imports warning.
  • Deno initialize workspace was run to add the settings.json.
  • Deno init was run to add a deno.json
  • "experimentalDecorators": true was added to compilerOptions in deno.json

Running deno test yields these results:

2024-05-06 12 28 37

Please verify these results by cloning:
https://github.com/GerbenRampaart/deno-nestjs-vanilla

This error is decorator related right?

@GerbenRampaart
Copy link

Ok, a bit weird. If I add "emitDecoratorMetadata": true to deno.json (which is not documented in the deno.json schema), it does work.

2024-05-06 12 50 57

2024-05-06 12 51 15

@GerbenRampaart
Copy link

@birkskyum @bartlomieju

2024-05-06 16 19 20

Integration test works, unit test works. Check the repo.

Further changes from vanilla nestjs project:

  • Removed prettier and lint dependencies, configs and scripts. Only using deno lint and deno fmt.
  • Removed package.json and moved dependencies to deno.json imports
  • Change the nestjs vanilla integration test to a Deno.test still using supertest.
  • Removed nest-cli.json

I think we can call this issue closed. Deno happily supports nestjs from the looks of it.

@birkskyum
Copy link
Contributor Author

Did you find a solution to the "TypeError: Class constructor Process cannot be invoked without 'new'" issue?

#20619 (comment)

@GerbenRampaart
Copy link

Did you find a solution to the "TypeError: Class constructor Process cannot be invoked without 'new'" issue?

#20619 (comment)

I didn't continue with the cloned variant of that typescript-starter of nestjs you started this issue with.

I figured that the @nest/cli also scaffolds a new vanilla nest project and with the most modern approach, the typescript-starter was initially committed a couple of years ago.

@birkskyum
Copy link
Contributor Author

birkskyum commented May 6, 2024

I tried making an new project with the cli approach instead just now, and I got the same TypeError: Class constructor Process cannot be invoked without 'new' error when running test.

@GerbenRampaart
Copy link

GerbenRampaart commented May 6, 2024

I tried making an new project with the cli approach instead just now, and I got the same TypeError: Class constructor Process cannot be invoked without 'new' error when running test.

Do you mind cloning my repo: https://github.com/GerbenRampaart/deno-nestjs-vanilla and share the output you get?

deno task test or deno test

and

deno task test:e2e

@birkskyum
Copy link
Contributor Author

birkskyum commented May 6, 2024

cloning my repo: https://github.com/GerbenRampaart/deno-nestjs-vanilla

These tests passed, both for test:e2e and test

So deno can run nestjs projects tailored specifically to deno, but it would be nice to be able to run existing projects with node compat.

@GerbenRampaart
Copy link

I think this is a good starting point for me @birkskyum, maybe your explicit goal was to make sure Deno executed the jest code explicitly, and not use Deno.test.

But honestly I'm not going to chase that dragon, the Deno.test looks very clean and the core issue for me was that nestjs was supported as platform/framework, with working dependency injection and so on, which it does.

@birkskyum
Copy link
Contributor Author

birkskyum commented May 6, 2024

That's reasonable - my focus was indeed on making initial adoption of deno frictionless, but using/migrating to the deno primitives like you do is ofc. ideal.

Is there an official starter for deno, like the one you shared?

@GerbenRampaart
Copy link

GerbenRampaart commented May 6, 2024

Is there an official starter for deno, like the one you shared?

If there is, I haven't found it. The nestjs people can be, with respect, ever so slightly stringent. If you open a Deno issue there it's closed before you can say 'ecmascript modu...'. I don't really blame them, they get mountains of questions and issues not caused by nestjs.

Regardless, I guess we're free to publish a Deno+Nest demo project for interested people to see anywhere, including somewhere in the Deno recipes in their docs.

@GerbenRampaart
Copy link

For reference, I did just stumble across this issue #20595 who's repo (https://github.com/mfulton26/deno-nest) draws some of the same conclusions I did. And they re-wrote the test using a BDD approach (like jest is):

https://deno.land/std@0.224.0/testing/bdd.ts

While I haven't pulled that repo and ran the test, I guess this is also an approach.

It still involves somewhat altering the test a bit (the import), but less so than Deno.test.

Maybe that's closer to your out-of-the-box support for jest-like testing @birkskyum .

@bartlomieju
Copy link
Member

Thanks for the updates @GerbenRampaart @birkskyum, happy to hear that NestJS is now somewhat usable with Deno!

I tried making an new project with the cli approach instead just now, and I got the same TypeError: Class constructor Process cannot be invoked without 'new' error when running test.

This error is gonna really painful to fix 😬 the problem seems to be that we use class Process and the code of interest is using ES5 syntax to instantiate, eg.:

if (!(this instanceof Process)) {
  Process.call(...)
}

I wonder if we should try to fix it upstream 🤔

@birkskyum
Copy link
Contributor Author

birkskyum commented May 7, 2024

I think it makes sense to fix jest-worker upstream, because it's so widely used. It's somewhat similar to this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node API polyfill Related to various "node:*" modules APIs node compat
Projects
None yet
Development

No branches or pull requests

4 participants