feat(schematics): add an option to generate nestjs node-app #807

FallenRiteMonk · 2018-10-10T12:29:10Z

No description provided.

FrozenPandaz

Thank you so much for creating this PR, it looks great! Just had a few points of feedback for you to address.

Also, please add e2e tests.

FrozenPandaz · 2018-10-17T21:54:41Z

packages/schematics/src/collection/node-application/index.ts

@@ -58,7 +77,7 @@ function createSourceCode(options: NormalizedSchema): Rule {
            tmpl: '',
            name: options.name
          }),
-          move(join(options.appProjectRoot, 'src'))
+          move(options.appProjectRoot)


I suppose this is alright if you need to add additional config.. but may I ask why this was needed?

I created the file the same way nest-cli created them and they create a test folder containing test next to the src folder, that's why. But I will revert this as you mentioned in the next comment that we do not need those tests.

FrozenPandaz · 2018-10-17T21:57:31Z

packages/schematics/src/lib-versions.ts

@@ -17,6 +17,10 @@ export const jasmineMarblesVersion = '0.3.1';
 export const expressVersion = '4.16.3';
 export const expressTypingsVersion = '4.16.0';

+export const nestjsVersion = '^5.1.0';
+export const supertestVersion = '^3.1.0';


I think we do not need supertest.

supertest is for end-to-end testing of APIs which we opt to test through the frontend application.

Example:
api1 is used by frontend1. Testing frontend1 e2e should cover testing api1 by definition. Thus, testing api1 "e2e" should not be necessary.

Will remove.

FrozenPandaz · 2018-10-17T21:59:10Z

packages/schematics/src/collection/node-application/files/nestjs/src/main.ts__tmpl__

+
+async function bootstrap() {
+  const app = await NestFactory.create(AppModule);
+  await app.listen(3000);


Can we use 3333 to match the express app?

FrozenPandaz · 2018-10-17T22:00:40Z

...s/schematics/src/collection/node-application/files/nestjs/src/app.controller.spec.ts__tmpl__

@@ -0,0 +1,22 @@
+import { Test, TestingModule } from '@nestjs/testing';


Not pertaining just to this file, but for these nest modules, controllers, and services: Can they go into the app directory similar to the Angular directory structure?

FallenRiteMonk

I have done the requested changes, but need some help with the e2e test (see comments)

FallenRiteMonk · 2018-10-18T08:57:25Z

e2e/schematics/node.test.ts

+        `
+      );
+      // const jestResult = await runCLIAsync('test node-app3');
+      // expect(jestResult.stderr).toContain('Test Suites: 1 passed, 1 total');


Having problems with this test. Help appreciated!

FallenRiteMonk · 2018-10-18T09:00:30Z

e2e/schematics/node.test.ts

+      expect(server).toBeTruthy();
+
+      await new Promise(resolve => {
+        server.stdout.once('data', async data => {


This only works because I disabled nest's default Logger.

With the default logger enabled, starting the server logs multiple lines, where only the last will be Listening at http://localhost:3333. How can I change this to have a short timeout and then get the last printed line?

FallenRiteMonk · 2018-10-18T09:02:02Z

packages/schematics/src/collection/node-application/files/nestjs/main.ts__tmpl__

+const port = 3333;
+
+async function bootstrap() {
+  const app = await NestFactory.create(AppModule, { logger: false });


{ logger: false } should be removed, but I need help to get the e2e test working properly with the default logger enabled, due to multiple lines being printed.

FrozenPandaz · 2018-10-22T21:05:16Z

@FallenRiteMonk

Thank you so much for taking the time to make this important contribution. We are sorry for not having a clear idea of how support of NestJS and other frameworks should look like earlier. Now that we have thought and discussed about how NestJS and other frameworks should be supported in Nx and we think the best approach is outside of this repository/ project for the following reasons:

Providing the best NestJS support means keeping the schematics up to date with the latest NestJS best practices which we believe the NestJS team is able to deliver better than we can. The NestJS team has the most expertise and the best idea for what best practices are good for NestJS. They would also be able to version the schematics along with the version of NestJS.
Our implementation of the node-app schematic with the option --framework none was designed to be extensible so it should provide a good base for writing schematics for NestJS and other frameworks.
We have reached out to @kamilmysliwiec with our ideas and he has expressed interest in updating the NestJS schematics and make them compatible with Nx/ AngularCLI projects!

@FallenRiteMonk
Once again, thank you for taking the time for this contribution and we are sorry for not being able to accept it. Please extend your efforts to help @kamilmysliwiec and the NestJS team be compatible with Nx. I am available for assistance if you need.

@kamilmysliwiec
Please connect with @FallenRiteMonk and establish a plan for updating the @nestjs/schematics to support Nx/ AngularCLI projects. Also, please let me know how I can help in anyway. Maybe you can create an issue in the appropriate NestJS project and mention us.

Please let me know what you think.

FallenRiteMonk · 2018-10-23T15:36:19Z

@FrozenPandaz I fully understand and appreciate your decision.

Thanks also for replying and not just letting this PR hang around without any comment.

Since I'm fairly new to creating/working with schematics and also not to deep into nestjs I don't know how much of a help I can be, but I would sure like to contribute as good as possible since I see great potential in the connection between nx and nestjs in some of my near future works, so @kamilmysliwiec pleas keep me up to date on what are the plans and how I can help. Thanks

beeman · 2018-10-23T19:48:54Z

For the people looking for an example on how to use Nest with Nx, check my starter project: https://github.com/colmena/angular-nest-universal-starter

I aim to support that project, so if it's not working in the future feel free to open an issue in that repo 👍

github-actions · 2023-03-23T10:07:37Z

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

FrozenPandaz requested changes Oct 17, 2018

View reviewed changes

feat(schematics): add an option to generate nestjs node-app

3e53951

FallenRiteMonk force-pushed the nestjs branch 2 times, most recently from c1d7b15 to 3e53951 Compare October 18, 2018 08:56

FallenRiteMonk commented Oct 18, 2018

View reviewed changes

FallenRiteMonk closed this Oct 23, 2018

skreborn mentioned this pull request Oct 24, 2018

Improve compatibility with Nx nestjs/schematics#105

Closed

FrozenPandaz mentioned this pull request Jan 3, 2019

feat(schematics): add node-app --framework nestjs #974

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(schematics): add an option to generate nestjs node-app #807

feat(schematics): add an option to generate nestjs node-app #807

FallenRiteMonk commented Oct 10, 2018

FrozenPandaz left a comment

FrozenPandaz Oct 17, 2018

FallenRiteMonk Oct 18, 2018

FrozenPandaz Oct 17, 2018

FallenRiteMonk Oct 18, 2018

FrozenPandaz Oct 17, 2018

FallenRiteMonk Oct 18, 2018

FrozenPandaz Oct 17, 2018

FallenRiteMonk Oct 18, 2018

FallenRiteMonk left a comment

FallenRiteMonk Oct 18, 2018

FallenRiteMonk Oct 18, 2018

FallenRiteMonk Oct 18, 2018

FrozenPandaz commented Oct 22, 2018

FallenRiteMonk commented Oct 23, 2018

beeman commented Oct 23, 2018

github-actions bot commented Mar 23, 2023

		@@ -0,0 +1,22 @@
		import { Test, TestingModule } from '@nestjs/testing';

feat(schematics): add an option to generate nestjs node-app #807

feat(schematics): add an option to generate nestjs node-app #807

Conversation

FallenRiteMonk commented Oct 10, 2018

FrozenPandaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FallenRiteMonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrozenPandaz commented Oct 22, 2018

FallenRiteMonk commented Oct 23, 2018

beeman commented Oct 23, 2018

github-actions bot commented Mar 23, 2023