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

feat(nest): implement nest lib schematic #2540

Merged
merged 15 commits into from
Mar 2, 2020

Conversation

yharaskrik
Copy link
Contributor

@yharaskrik yharaskrik commented Feb 24, 2020

Current Behavior (This is the behavior we have today, before the PR is merged)

Can only generate nest apps

Expected Behavior (This is the new behavior we can expect after the PR is merged)

Can now generate a nest library with optional service, controller and @global module

Closes #1010
Closes #2378

- add flags for adding a service and/or a controller to the module
- add flag for including the
Global decorator on the Nest module
Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

Looks good! I suggest you try to use the @nrwl/node:lib schematic instead of the workspace one. The node one includes a lot of the handling for publishing and creating tsconfig files. You can massage the files the node:lib schematic creates if you need to.

Also, can you please update the shared-new.ts file here to include the nest library:

json.schematics['@nrwl/nest'] = { application: { linter } };

packages/nest/src/schematics/library/library.spec.ts Outdated Show resolved Hide resolved
packages/nest/src/schematics/library/library.ts Outdated Show resolved Hide resolved
create the --target flag for choosing what es target is added to the generated tsconfig.json, nest
libraries will default jest configs to testEnvironment=node instead of defaulting to jsdom. jsdom is
still default for other schematics
@yharaskrik
Copy link
Contributor Author

@Cammisuli I made your requested changed and added a couple of other things, defaulting the es target to es6 for nest libraries (as suggested by Kamil) and setting the testEnvironment in the jest.config.js to 'node' rather than jsdom since we are not using nest libs in a DOM environment and if anyone is using mongoose than mongoose suggests using the 'node' env rather than 'jsdom' env.

#1002
https://mongoosejs.com/docs/jest.html

@yharaskrik
Copy link
Contributor Author

check-imports.js also failed because I am importing from angular devkit (the stripIndents function) but apparently that is not allowed?

Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

Please remember to update

json.schematics['@nrwl/nest'] = { application: { linter } };

Also, I just realized that e2e's are needed for the nest changes (in node.test.ts) and the jest changes.

scripts/commit-lint.js Outdated Show resolved Hide resolved
packages/nest/src/schematics/library/library.spec.ts Outdated Show resolved Hide resolved
packages/nest/src/schematics/library/library.spec.ts Outdated Show resolved Hide resolved
packages/nest/src/schematics/library/library.ts Outdated Show resolved Hide resolved
@yharaskrik
Copy link
Contributor Author

Thanks for the feedback @Cammisuli I could have sworn I updated that shared-new.ts file but maybe I daydreamed that.

@yharaskrik
Copy link
Contributor Author

@Cammisuli I am encountering a strange situation with the e2e tests. When I run the e2e tests that I wrote they fail linting with:

Linting "nestlib9652709"...
    
    ERROR: /Users/jaybell/WebstormProjects/nx/tmp/nx/proj/libs/nestlib9652709/src/index.ts[4, 1]: unused expression, expected an assignment or function call
    ERROR: /Users/jaybell/WebstormProjects/nx/tmp/nx/proj/libs/nestlib9652709/src/index.ts[4, 7]: unused expression, expected an assignment or function call
    
     Lint errors found in the listed files.

But this expect passes in the unit tests

const barrel = getFileContent(tree, 'libs/my-lib/src/index.ts');
      expect(stripIndents`${barrel}`).toEqual(
          stripIndents`export * from './lib/my-lib.module';
          export * from './lib/my-lib.service';
          export * from './lib/my-lib.controller';`
      );

I am not sure how both of those cases can be true, if you have some input that would be awesome.

Also, would you like me to split these nest tests out into their own e2e file? Or leave them in with node? As the nest schematics grow it might make the most sense to split them out since @nrwl/nest is a different package than @nrwl/node?

I still need to write the jest e2e tests but everything else should be good now.

Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

Don't worry about splitting the e2e file. I'll do that sometime next week.

@yharaskrik
Copy link
Contributor Author

Ok sounds good. I would like to add more in the future to nest (generated integration testing, guards, interceptors etc) so don't want that file to get crowded.

user propertyName for injecting Service into Controller
update spec files to include imports
test
hest config was generated with testEnvironment: 'node'
@yharaskrik
Copy link
Contributor Author

@Cammisuli what would be the best way to load the value of the jest.config.js into memory to check the testEnvironment in the e2e test? Is there a util function I can use to load it into a json object?

@Cammisuli
Copy link
Member

Cammisuli commented Feb 28, 2020 via email

…receives it

allow testEnvironment to be customized in @nrwl/nest:lib, @nrwl/node:lib and @nrwl/workspace:lib if
using unitTestRunner=jest
@Cammisuli Cammisuli self-requested a review March 2, 2020 14:37
Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@yharaskrik
Copy link
Contributor Author

Thanks! I did implement some of the other main nest features as well in another PR that refractors some of the code in this PR as well as splits service and controller out so they can be generated independently.

@Cammisuli
Copy link
Member

We'll keep them separated. Let's get this merged in, run some smoke tests to make sure everything is good, and then get the next one in.

@yharaskrik
Copy link
Contributor Author

Sounds good! Figured that's how we would do it. The other has quite a bit of changes and I realistically could write more unit tests as there are a lot of different combinations of flags now for a lib

@Cammisuli Cammisuli merged commit a39587a into nrwl:master Mar 2, 2020
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Nest library schematic NestJS: Support creating libs.
2 participants