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

feature() extract platforms (express/fastify/socket.io/ws) #1329

Merged
merged 5 commits into from Dec 29, 2018

Conversation

@kamilmysliwiec
Copy link
Member

commented Nov 30, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: #531

What is the new behavior?

First iteration to extract HTTP adapters (WIP)

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

? [serverOrOptions, options]
: [ExpressFactory.create(), serverOrOptions];
: [this.createHttpAdapter(), serverOrOptions];

This comment has been minimized.

Copy link
@BrunnerLivio

BrunnerLivio Nov 30, 2018

Member

Is this just temporarily or will the core in 6.0.0 still contain a HTTP server?

This comment has been minimized.

Copy link
@kamilmysliwiec

kamilmysliwiec Dec 1, 2018

Author Member

This is basically an implementation of this idea so far: #531 (comment)

  • people no longer have to ship all this express, body-parser (HTTP stuff) unless they need them
  • @nestjs/platform-express is loaded lazily which gives a default option for everybody
  • it doesn't break the public API so all tutorials, videos, courses, examples are still up-to-date
return new ExpressAdapter(httpAdapter);
private createHttpAdapter<T = any>(httpServer?: T): HttpServer {
const { ExpressAdapter } = loadPackage(
'@nestjs/platform-express',

This comment has been minimized.

Copy link
@BrunnerLivio

BrunnerLivio Nov 30, 2018

Member

This will lead to a circular dependency. Should the core really depend on platform-express?

@marcus-sa

This comment has been minimized.

Copy link

commented Dec 1, 2018

Why make it a factory?
You should make it a module you import lol

Server module with a forRoot method that requires 2 parameters, an adapter and options for that adapter.

And then you'd have a method forFeature that is used to specify the controllers you'd want along with a route configuration.

@BrunnerLivio

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

I gotta be honest, I agree on @marcus-sa proposal. I've already suggested a similar idea here. In my opinion refactoring this in a module will lead to a cleaner code base and will definitely benefit the framework in the long run. @DanielSchaffer framework does this similar.

@marcus-sa

This comment has been minimized.

Copy link

commented Dec 1, 2018

This is an example of how I'd suggest it
https://github.com/marcus-sa/nest/tree/feat/one/examples/platform-express/src
Never ever add non core features which modifies the behavior and instantiation of Nest.
Everything should be coded as an importable module, because that is the overall idea of a "module architecture framework"

@kamilmysliwiec

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

I'll quote my post once again:

Nest was designed not to be a copy of Angular(!), but also to stay as close to express as possible. This library is used across majorities of Node.js apps and it was a very important goal to make the transition smooth for most of the people. The target was always to become a web-platform and at the same time, to keep codebase platform-agnostic and flexible (but still with keeping in mind that web is the biggest target, the most essential one and steps to start building web app should be very easy, pretty straightforward).

Nest was never meant to, for example, run in the browser and DI/module system is not the only thing that should be considered as a "core" one. I do respect your ideas, I understand them, but being express compatible was a very important goal, it was a primary design decision and building web apps/apis is the most important feature of the framework. This feature shouldn't require any additional, ancillary steps, importing various modules and stuff like that. Current PR makes that HTTP-related packages don't have to be shipped with each existing Nest application anymore. However, it still treats web APIs, express/fastify compatibility very seriously and that is definitely a long-term plan.

And beyond that, I'm a strong opponent of introducing things that break API for no reason and without any visible benefit for people and at the same time making all tutorials, videos, courses, examples, things that people have built so far - outdated, bringing only confusion and troubles to people. Certainly, spliiting all things into separated (and then manually imported) modules would make things more loosely coupled, might seem to be cleaner from the perspective of the fully modular framework that only sits here to serve DI etc, but it's not the purpose of Nest.

@DanielSchaffer

This comment has been minimized.

Copy link

commented Dec 1, 2018

This feature shouldn't require any additional, ancillary steps, importing various modules and stuff like that

I'm a bit confused as to where this idea is coming from. The JavaScript ecosystem is rife with counterexamples. With Angular, you need at least 4-5 packages for anything but the simplest of applications. Webpack on its own is mostly useless - you need loaders and plugins for anything other than pure ES5 source. Even with express itself, you need utility packages in order to provide what most would consider "core" functionality - for example, body-parser, cookie-parser, cors, compression ... the list goes on (no, really, it does).

There are multiple benefits to this pattern of modularization, generally revolving around the overarching concept of KISS:

  • Developers can decide for themselves which functionality they need. Yeah, nobody's going to be using Nest in the browser. But with TypeScript increasing in popularity as an option for writing NodeJS applications, it is entirely likely that someone would want to write a server application that does not have an HTTP interface. DI is not a concept specific to HTTP services, and this use case is something Nest could likely very easily work for.
  • It's easier to test and maintain smaller pieces of code. Sure, this isn't a benefit that developers would see directly - but keeping packages smaller and more focused tends to result in an overall more stable architecture.
  • Smaller packages are easier to extend as well. For example, in my Dandi framework, I also have model validation and binding packages. The great thing about the way I implemented it is that you can actually share the models between the server and the web client, because I designed the package hierarchy such that the modeling types and interfaces are separate from most everything else in the framework. This means I can have my @dandi/model package as dependency of my angular app without being forced to bring along all sorts of other random dependencies.

None of this means that the primary use case of Nest would change, or that integration with express or fastify has to be any less important. To me, these changes are completely orthogonal to those concepts: this about better architecture, better maintainability, better extensibility, and generally, more future-proof.

Developers are generally pretty smart people. Especially considering the architectural trends in the JavaScript and TypeScript communities lately, needing to add one or two more packages is not going to throw our collective worlds into chaos. It is true, breaking changes should be avoided unless there is a clear benefit, and it is my opinion that this absolutely meets that criteria. Things like this are what major semver versions are for.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

But with TypeScript increasing in popularity as an option for writing NodeJS applications, it is entirely likely that someone would want to write a server application that does not have an HTTP interface. DI is not a concept specific to HTTP services, and this use case is something Nest could likely very easily work for.

Nobody says that DI is a concept specific to HTTP services. With Nest, you can build anything that might be normally created on top of Node (like CLI, Jobs, basically everything). https://docs.nestjs.com/execution-context

@DanielSchaffer

This comment has been minimized.

Copy link

commented Dec 1, 2018

With Nest, you can build anything that might be normally created on top of Node (like CLI, Jobs, basically everything).

Right - as it should - but your core package still has a direct dependency on express, body-parser, cors ... these are useless and unnecessary unless you are writing an HTTP service. A well written HTTP service is a service with an HTTP interface attached to it, not a service directly exposed to HTTP. It'd be helpful for the framework to mirror that hierarchy.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

but your core package still has a direct dependency on express, body-parser, cors ... these are useless and unnecessary unless you are writing an HTTP service.

This is essentially what this PR has fixed (all these packages aren't going to be shipped anymore).

@@ -20,9 +20,7 @@
},
"dependencies": {
"@nuxtjs/opencollective": "0.1.0",
"body-parser": "1.18.3",
"cors": "2.8.4",

This comment has been minimized.

Copy link
@DanielSchaffer

DanielSchaffer Dec 1, 2018

This can be moved to the express package as well.

@DanielSchaffer

This comment has been minimized.

Copy link

commented Dec 1, 2018

@kamilmysliwiec My apologies - my notifications got mixed up and I thought this was a different discussion thread. Anyhow, there's still a stray cors dependency: #1329 (comment)

@kamilmysliwiec

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

@DanielSchaffer yeah, I haven't finished yet 😄 Just pushed an update.

@coveralls

This comment has been minimized.

Copy link

commented Dec 1, 2018

Pull Request Test Coverage Report for Build 1286

  • 14 of 14 (100.0%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 93.799%

Totals Coverage Status
Change from base Build 1282: -0.008%
Covered Lines: 2819
Relevant Lines: 2940

💛 - Coveralls
@coveralls

This comment has been minimized.

Copy link

commented Dec 1, 2018

Pull Request Test Coverage Report for Build 1326

  • 26 of 26 (100.0%) changed or added relevant lines in 18 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 93.78%

Totals Coverage Status
Change from base Build 1319: -0.005%
Covered Lines: 2845
Relevant Lines: 2967

💛 - Coveralls
@marcus-sa

This comment has been minimized.

Copy link

commented Dec 1, 2018

Actually yes, I would use Nest in the browser if it wasn't server specific.
I had to write my own MAF because both Angular and Nest have limitations and a really poor API for creating 3rd party modules.

Angular? Unless you need a template engine with component logic, you can't use their framework.
Nest? Unless you need controllers and specific server based features, you can't use Nest.

I wanted to use the same architecture for our browser extensions at work, but that was impossible, due to the limited extensibility Angular & Nest provided.

So you're wrong at that point 😁

@DanielSchaffer

This comment has been minimized.

Copy link

commented Dec 1, 2018

@marcus-sa ha, well I suppose I stand corrected. Perhaps “nobody” is too much of a blanket statement - but my point was more that the fact that web-based usage is more of an edge case, isn’t really an excuse to avoid modularity. In other words, holding up “but most usage is not web-based” is a bit of a straw man, because the argument is about good architecture, and while web-based usage would be made easier, that’s not the purpose.

@kamilmysliwiec kamilmysliwiec changed the title [WIP] feature(core/common) extract HTTP adapters (express/fastify) [WIP] feature() extract platform (express/fastify/socket.io/ws) Dec 12, 2018

@kamilmysliwiec kamilmysliwiec changed the title [WIP] feature() extract platform (express/fastify/socket.io/ws) [WIP] feature() extract platforms (express/fastify/socket.io/ws) Dec 12, 2018

@kamilmysliwiec kamilmysliwiec changed the title [WIP] feature() extract platforms (express/fastify/socket.io/ws) feature() extract platforms (express/fastify/socket.io/ws) Dec 29, 2018

@kamilmysliwiec kamilmysliwiec merged commit 856141d into 6.0.0-next Dec 29, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.005%) to 93.78%
Details

@delete-merged-branch delete-merged-branch bot deleted the feature/extract-http branch Dec 29, 2018

@kamilmysliwiec kamilmysliwiec referenced this pull request Feb 11, 2019
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.