use yarn workspace to build packages

[xcaptain]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

I rebased my original pull request in #954 and send this new pull request, changes includes:

1. use yarn workspace to build packages
2. deleted the bundle directory to exclude the compiled files from the repo
3. included sample directory into workspace to use the latest nest library in sample

how to test this pr

1. clone this repo. git clone https://github.com/xcaptain/nest
2. cd nest
3. git checkout feature/workspace
4. yarn install this will install all the dependencies in packages and sample
5. yarn bootstrap to bootstrap lerna
6. after made any changes run yarn prepare to re-generate lib in each packages
7. yarn test to run all the tests or ../../node_modules/mocha/bin/mocha --require ts-node/register test/**/*.spec.ts in each packages
8. yarn pub to publish the packages, this command will publish the source code and compiled code

how to run the sample

1. cd sample/01-cats-app/
2. yarn start
3. curl -i 'localhost:3000/cats/

what to do in the future

• put all the source code into a src directory in each packages and compiled into lib dir
• delete unnessary dependencies in sample dir
• try typescript 3 project reference feature

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[marcus-sa]

When using workspaces, you should structure the project differently.
Each package should have a src directory, and then from that packages root directory, there should be a package.json where all the workspace packages being used will be included as a dependency, similar tsconfig.build.json and a tsconfig.json that extends the top root tsconfig.
I'd suggest setting it up the same as this https://github.com/Quramy/lerna-yarn-workspaces-example

[xcaptain]

@marcus-sa I know it's good to separate each package, current package structure comes with some problems.

1. Code circular dependencies, @nestjs/common depends and @nestjs/core and @nestjs/core depends @nestjs/common

2. Inappropriate API structure, every public API should be defined in index.ts, but current we have lots of code like import { RuntimeException } from '@nestjs/core/errors/exceptions/runtime.exception';

I think my current commits is a minium modification towards yarn workspace and can be merged. I will try to work this 2 problems out this weekend.

[marcus-sa]

@nestjs/common is obsolete and shouldn't even have been there in the first place.
Simply move everything from there to @nestjs/core

[marcus-sa]

Breaking changes, who cares? Meanwhile you're at it you could remove all the deprecated features anyway, I mean if they're not gonna be removed, why tf even mark them as deprecated?

[xcaptain]

I put source code into an src folder in @nestjs/common and @nestjs/core package, and I have changed all the relative import in test into absolute import in @nestjs/common.

It's such a boring thing, I don't know if these changes are good for you @kamilmysliwiec

[xcaptain]

Current status:

All the 5 packages compile well, and sample code runs well, so this should be a backward compitable change.

This package structure looks really good 😸

Current package list:

I'd like to move sample directory into workspace so we can try the latest code in sample projects. package-lock.json and yarn.lock file are no longer needed in the sample projects, because they will share the root yarn.lock.

Current test status:

I just fixed test case in @nestjs/common, because change from relative import to absolute import is so boring and need to change so many code, If people like this change I can do more.

Advice and reviews are welcome, thanks!

[BrunnerLivio]

@xcaptain Great job!! Really well done!
One minor thing; could you update the .travis.yml so it does not run npm ci but yarn instead? Seems like Yarn does not have any ci mode yet: yarnpkg/yarn#5869, so I think we should just simply use the command yarn

[xcaptain]

All the import statements in test dir have changed, the old import style includes:

1. in package relative path reference, like ../utils/
2. across package relative path reference like ../../../common/constants
3. package name with path, like @nestjs/common/constants

I have uninformed all these styles into 1 import {} from '@nestjs/common' style, this means only public API defined in src/index.ts can be tested, I think this is much better.

Till now I think this pr is ready to be reviewed and merged.

[kamilmysliwiec]

The only problem which I notice is that this PR is incredibly huge which makes it almost impossible to review

[tonivj5]

Maybe removing the bundle directory?

[kamilmysliwiec]

Maybe is there a way to somehow split this PR into a few parts? They don't have to necessarily play well separately. For example, firstly just push configuration stuff+small instructions what should I do in order to make it work well. I could move all source files manually which would be 1000x faster than reviewing each existing file 😅

[whtiehack]

@kamilmysliwiec This is only project struct adjust ,there are not any function changes.

[xcaptain]

@kamilmysliwiec This pr should be backward compatible, the main work I did includes:

1. add packages and sample directory into yarn workspace
2. put all the source code in each package into a separate src directory
3. fix tests in each package because step 2 makes most tests fail

tests in @nestjs/common, @nestjs/core, @nestjs/websocket works well, only @nestjs/microservices has some fails. I think if tests passed and some sample project works this pull request can be considered right.

[xcaptain]

Yes, this pr was mainly just directory structure change, this idea originally come from that I want to learn this framework but after I dug into the source code I found that I have to install these packages from npmjs rather than local. If the pr get merged I think contributing to this project will be much easier

[kamilmysliwiec]

Please, could you get rid of merge conflicts? Looking at the source code now :)

[xcaptain]

@kamilmysliwiec I just rebased this pull request. I think the biggest controversial point exists in how I deal with module import in test dir, I exposed a lot of API in index.ts to make tests run.

[kamilmysliwiec]

Just figured this out. I think that the problem really is that this PR simplifies contribution (which is great) although it affects developer experience. If we decide to export everything from the root file, it would mean that each of these interfaces/classes is a part of the public API that may be used by the developers in their projects built on top of Nest. We definitely cannot export everything, it may bring a lot of mess in auto-completion/intellisense provided by the IDE. A huge list of useless exports may potentially lead to very bad decisions.

[kamilmysliwiec]

Some of this stuff is used by nest packages internally, just to provide more context about a specific thing. They should never be used in real projects.

[xcaptain]

@kamilmysliwiec Yes, I know expose too much API is bad, but as I said in #1060 (comment) I recgonize test a separate package along with src, so every method or class used in test can be public in src.

For example @nestjs/common should be a blackbox to @nestjs/core/test, so we should write import SomeAPI from "@nestjs/common".

To import in @nestjs/core we have 2 ways to do:

1. import SomeAPI from "@nestjs/core"
2. import SomeAPI from ../src/...

But for style consistency I prefer to write the former one.

[kamilmysliwiec]

I understand it @xcaptain. However, we still need to get rid of internal types and it's impossible with the current approach.

[kamilmysliwiec]

Good practices and code organization are fine as soon as they don't mean a lot of troubles for the end user.

[xcaptain]

Ok, I will revert these files and then fix tests using relative path reference.

[marcus-sa]

Not that hard to fix.
Simply do it like this:

{
  "extends": "./tsconfig.base.json",
  "compilerOptions": {
    "baseUrl": "./packages",
    "paths": {
      "@nestjs/core": ["./core/src"],
      "@nestjs/core/*": ["./core/src/*"],
      "@nestjs/common": ["./common/src"],
      "@nestjs/common/*": ["./common/src/*"]
    }
  },
  "exclude": [
    "./packages/*/__tests__",
    "./packages/*/lib",
    "node_modules"
  ]
}

Only downside is that you'd have to add two paths per package.
Then you don't have to export anything unrelated to the users from the root and can use full path module aliases such as

import { NestedFile } from '@nestjs/core/nested/dir/whatever/path';
import { RootFile } from '@nestjs/core';

or you could just keep it as it currently is, and use the base url.
this is inconvenient for the contributor though

// <rootDir>/packages/core/src/nested/dir/whatever/path.ts
import { NestedFile } from 'core/nested/dir/whatever/path';
// <rootDir>/packages/core/src/index.ts
import { RootFile } from '@nestjs/core';

[xcaptain]

@marcus-sa Path mapping is good but this makes all the exported APIs accessible outside the package which I think @kamilmysliwiec won't like because of exposing too many public APIs.

[marcus-sa]

Put sample directory under packages in lerna.json and register the module aliases using nodemon --watch ../../packages --watch ./src -x ts-node -r tsconfig-paths/register src/index.ts in the script commands

[kamilmysliwiec]

Not sure if sample inside packages is a good decision(?)

[marcus-sa]

https://github.com/marcus-sa/one check my repo, it works flawlessly (that'll soon be a PR for nest once I've migrated the server related modules)

[kamilmysliwiec]

I believe that it works, I'm rather thinking about whether it's straightforward from the user perspective (to lookup for samples in the packages dir)

[kamilmysliwiec]

(that'll soon be a PR for nest once I've migrated the server related modules)

@marcus-sa ❤️:boom:

[marcus-sa]

You don't have to lookup samples in the packages directory.
You can have multiple directories under the packages array in the lerna config as I've done.

[kamilmysliwiec]

Ohh, I thought you were talking about directories tree. Sorry for the confusion, this is the outcome of reading email's notifications 😅

[xcaptain]

This pull request is still not ready to merge due to microsoft/TypeScript#10866, though src compiled successfully and the test cases passed, the compiled lib can't be used directly. When trying to build sample code I always come across errors like Error: Cannot find module '@nestjs/common/http/http.constants', actually require('@nestjs/common/lib/http/http.constants') works fine.

I think this is an typescript bug but I don't know why it's not solved in such a long time.

[marcus-sa]

That is not a TypeScript bug, that's just how module resolution works in Node.

[xcaptain]

@marcus-sa So do you have any good solution on this, for example:

in packages/core/lib/adapters/fastify-adapter.js contains const load_package_util_1 = require("@nestjs/common/utils/load-package.util"); but this require would fail because the actual require path is @nestjs/common/lib/utils/load-package.util

[kamilmysliwiec]

Just a quick note - moving output files to the lib directory inside each package would break each feature package (like graphql or swagger) that depends on Nest internal-specific stuff (for example MetadataScanner)

[xcaptain]

The only solution I have is to output the lib into the bundle, So again we can't delete bundle, seems doing nothing these days 😭

[marcus-sa]

Only solution without having to access the lib directory, would be to put everything into the published package directory like lodash does

You could copy package.json from the package directory, to the lib directory, along with the compiled files, so they're all in the same directory, and then point lerna to publish that specific directory as it is.

[marcus-sa]

+-- packages
|   +-- core <-- instead of publishing this directory
|       +-- lib <-- publish this
|           +-- package.json
|           +-- nest-factory.js
|           +-- index.js
|           +-- ...
|       +-- src
|           +-- nest-factory.ts
|           +-- index.ts
|           +-- ...
|       +-- package.json
|       +-- tsconfig.json
|       +-- tsconfig.build.json
|   +-- ...
+-- .gitignore
+-- .npmignore
+-- .prettierrc
+-- package.json
+-- lerna.json
+-- tsconfig.json
+-- tsconfig.base.json
+-- yarn.lock
+-- ...

then tell lerna to publish the lib directories, since it contains the copied package.json

[xcaptain]

There was a node issue about this nodejs/node#15755 but no one cares. @marcus-sa lerna won't support publish specific directory

[marcus-sa]

There's always those small hacks that makes life great sometimes ;)
You can tell lerna to only publish a directory path, so basically you could write a publish.js script, that travels through the lib directories and runs lerna publish or yarn publish one by one.

Steps

1. Version patching using lerna
2. Compile each package to their respective lib directory using yarn run lerna run build
3. Copy package.json from package directory into lib
4. Run yarn publish alongside git commit -m "chore whatever" you name it.

Could easily be done using a shell script instead.

[BrunnerLivio]

General question regarding this topic;

It seems like the problem is certain packages are using internal functionalities of different nest packages which are not part of their public API?
Is this how it should be done? Shouldn't Nest packages also only be able to consume the public API of other Nest packages?

I am not an Angular expert; but does anybody know how they solve this issue?

Reading through your suggestions it feels like we're just creating a workaround to get it working, but not solving the real issue?

[kamilmysliwiec]

@BrunnerLivio
The most important factor will always be the developer experience. Intellisense, code auto-completion, they have to work well and properly indicate what is a part of the public API and what is not. I won't expose internal features which are used only by the 3rd party packages (mostly these more advanced ones which are considered as an "official ecosystem" like e.g. swagger) just to fulfil requirements of "how things should look like" because I don't care about it too much as soon as the people feel comfortable and it gives them better experience and certainty what they should use to quickly build their apps :)

[BrunnerLivio]

@kamilmysliwiec
I did not mean to expose internal API to everyone, that is off course a bad idea. But I question if things like MetadataScanner or loadPackage are supposed to be in a @nestjs/core package? Maybe they should be their own npm module e.g. npm install load-package? I think every @nestjs/* package should be handled as a blackbox, even for internal packages.

[kamilmysliwiec]

I don't think that splitting the package into even more packages gives any substantial value, to be honest.

[xcaptain]

I tried 2 ways to implement this pull request

1. Put api used by other package source into index.ts, which means if @nestjs/core want to use an api in @nestjs/common, this api must be defined in common/src/index.ts. I think this idea is ok, the mistake I made is extracting apis used by test into index.ts which causes too many files changed.

2. use path mapping, this way makes fewer changes but the compiled lib can't be used directly.

Personally I prefer the first way, only one rule "if an api used by another package, this api must be public and must be defined in index.ts", and I think that's how angular does.

This picture shows in angular's code base, packages/common/src, packages/core/src, packages/router/src, packages/http/src don't contains @angular/somepkg/somepath style import statement.

[xcaptain]

Angular uses path mapping but only in test

Maybe we can make a tradeoff, for those projects wide public api, they have to be defined in index.ts and inner public api can be invoked through path mapping

[kamilmysliwiec]

Personally, everything is fine as soon as we fulfill 2 requirements: (1) don't expose everything to the user for better UX (2) don't break API of 3rd-party libs 🙂 src directories look good but they break the second one(?) I believe

[kamilmysliwiec]

Just merged #1176 which should solve 99% of issues

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.