feat(@nestjs/core): execute middleware of dependent modules first. #1451

[underfin]

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
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: 1451

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[coveralls]

Pull Request Test Coverage Report for Build 3231

• 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 94.96%

---

Totals
Change from base Build 3166:	0.007%
Covered Lines:	3467
Relevant Lines:	3651

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 3490

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.009%) to 95.16%

---

Totals
Change from base Build 3467:	0.009%
Covered Lines:	3539
Relevant Lines:	3719

---

💛 - Coveralls

[kamilmysliwiec]

What if the single module is imported by multiple modules that will override distance value several times?

[underfin]

public addRelatedModule(module: Module) {
    this._imports.add(module);
    module.distance = this._distance + 1 > module._distance ? this._distance + 1 : module._distance;
  }

The single module distance will check distance when it is imported by multiple modules.If the distance less than the distance that imported module + 1, it will be updated.

[kamilmysliwiec]

I left a single comment. Could you look at it?

[kamilmysliwiec]

Could you try running the integration tests? (more info here https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md)

Lots of them are failing now (perhaps due to recursion - what if there is a circular dependency between modules?)

[underfin]

Sorry, I get some errors when I run integration test, but it is no related with this, it is caused by mocha config.So I ignore it.The error with this, it is caused by scanner the test files in node_modules.

/Users/likui/code/github/nest/integration/graphql/node_modules/@nestjs/graphql/tests/e2e/generated-definitions.spec.ts:1
(function (exports, require, module, __filename, __dirname) { import * as path from 'path';
                                                                     ^

I already fix it by change glob path in mocha command.

    "integration-test": "mocha 'integration/*/!(node_modules)/*.spec.ts' --reporter spec --require ts-node/register --require 'node_modules/reflect-metadata/Reflect.js'"

If it is an issue, I can pull a pr fix it.

[underfin]

The current code ignored circular dependency between modules. I am not an idea for this, could you give me some suggestion?
Maybe we can provide a way with middleware order decorator, like @MiddlewareOrder.But it is a problem when middleware number is larger, the management with order maybe chaotic.

[underfin]

Maybe we can ignore the middleware order with circular dependency between modules,only run middleware in order with normal dependency.

[kamilmysliwiec]

Maybe we can ignore the middleware order with circular dependency between modules,only run middleware in order with normal dependency.

That would make sense :)

[underfin]

Because the order with circular dependency between modules is not clear, so I think ignore this sence, the order with this inited 0 and dont change.But the order with normal dependency is clear, I think only support it. The loop with circular dependency between modules update distance` is already fixed in my last commit.

[gelito]

Can I ask the logic behind this change??

Example:

app.module
--> apply MiddlewareA
--> imports b.module

b.module
--> apply MiddlewareB

The execution logic said that MiddlewareA must be executed first than MiddlewareB because the app.module is loaded first than b.module.

And, on the other side, app.module will have the most generic middlewares, so will be executed first, but now, we don't have an easy way to have a "generic and first" middleware.

So, why is the logic behind invert the order? Or Am I losing something?

[kamilmysliwiec]

@gelito I'm so sorry for this regression, you're totally right!! The middleware execution order should be inverted, and these modules closer to the root module (or in the root module, e.g. app.module) should be applied first! I have just published 6.6.3 version that fixes this issue :)

[gelito]

Thanks so much!

I think the logic must be this in all modules; in our case, for example, we do:

App.module.ts
--> import User.module.ts

User.module.ts
--> route: \users\:userHash
--> user.middleware: capture the :userHash (if exists) and save it into req
--> user.decorator: Take the req.model.user if exists
--> import Cars.module.ts

Cars.module.ts
--> route: \users\:userHash\cars\:carHash
--> car.middleware: capture the :carHash (if exists) and save it into req.
--> controller: with the @user() decorator access to the user.

And so on...

We think it's the more logical way, but we are always open to seeing alternatives ;-)

[asheliahut]

So the inversion that was done was to revert it to the original order? as changing the execution of middleware is a breaking change in my mind.

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