Open erikkrieg opened 5 years ago
I also just wanted to thank the contributors of this project for working towards addressing the lack of strong convention in the Node server-side ecosystem ❤️
@erikkrieg Please review an example in https://github.com/nestjs/nest/pull/1573 After this is polished and merged, same changes could be applied to Nest CLI as well.
Thank you @erikkrieg.
Just to explain. Using ConfigService
outside of the application context is possible, but is not (always) considered as a best practice. Basically, ConfigService
should be used within your application so you can easily mock/override it in your tests. Nonetheless, there is nothing wrong with using process.env
directly in your main.ts
which you will very likely don't test at all.
I am unsure why the issue has been closed. Might be a misunderstanding on what I was trying to communicate, or perhaps something I am missing about the CLI.
I don't really know this framework well enough to have an opinion on how this project handles configuration so I referred to the ConfigService
class because that is what is used in the docs to showcase this best practice.
To clarify the issue I was pointing to, my concern is that the CLI creates a new project that lacks a good convention for handling configuration. In this case, the port that the server listens on. This implicitly promotes an anti-pattern and misses an opportunity to share the opinions that project already seems to have on how to handle configuration.
The Node ecosystem is sorely missing a strong, opinionated framework like what Rails is to Ruby. I hope that this project is aspiring to fill that void. I think having the CLI get devs started without having to think about setting conventions like this helps get there.
@erikkrieg
In this case, the port that the server listens on
Well, that might be a good thing actually. I thought you wanted to suggest the usage of the ConfigService
explicitly. I think that we could change the schematic, so it will generate this:
app.listen(process.env.PORT || 3000)
(instead of just hardcoded port)
Yeah, that is a step in the direction I was trying to communicate. Ofc it is up to the maintainers to determine how sophisticated the handling of config in the "basic" project generated from the CLI should be.
Hey firstly @kamilmysliwiec, great job on this project; I'm enjoying working with it :).
Using something like app.listen(process.env.PORT || 3000)
assumes that the env variable is configured and available for this app. Considering this schematic is likely to be used for creating multiple projects we will likely still need to end up changing this.
I've happened to solve this for our custom schematic that extends this in order to add some business specifics.
By utilizing environments files(As Angular does) and schema properties in combination, we are able to generate and run multiple nest apps straight from the cli with no additional config. I think something along these lines could provide an elegant solution.
Example:
We set the environment.prod file to expose a server_port property to a environment variable.
export const environment = { production: true, server_port: process.env'<%=name%>-port'] };
And in the dev environment file we used a port chosen at the generate stage.
export const environment = { production: false, server_port: '<%=port%>' };
The schema.json sets the default and prompts for a choice
"port": { "description": "The dev port this server should run on.", "type": "string", "default": "3333", "x-prompt": "What port would you like to use for the app?" },
In our main.ts we just import import { environment } from './environments/environment';
and use it like app.listen(environment.server_port, () => {});
Actually, there is a popular npm package (https://github.com/lorenwest/node-config) that allows you to define config files for different "environments" and at runtime it will pick the config matching NODE_ENV
. I think this sufficiently abstracts the configuration process and makes it fairly easy to switch environments (see the README examples). As @kamilmysliwiec noted, it is fine that this aspect of the configuration mechanism is not provided as a service since the bootstrap()
is out of the DI-scope anyways. Of course, nonetheless, nothing prevents you from wrapping the config mechanism in a service to provide it to other parts of your app later.
@felixhayashi yes this works ....thank you so much, especially the fact that node-config has a feature of
Custom Env variables
It took me a while to realise that I need to keep the config
folder outside the src
folder and then it worked beautifully. Thank you
Many people use .env but I have been not saw who think about why we should use .env at first time and why there are people that use as .env.bak
.
Generally sensitive data should be not exposed in some file or and should be not tracked.
Of source, I can understand always setting vars is to be tedious too (or maybe we should patch encrypted vars dynamically with big working). But tracking or maintaining env vars on source, it means injector is not only one like pm2 ecosystem json or something in future. then it also means config service cannot be fixed.
Moreover, Nowadays on javascript, pm2 is used on dev / staging env normally.
Maybe you don't need to use .env except on local env. Finally I think, it is ok this issue is closed.
export class AppModule {
static port: string
constructor(configService: ConfigService) {
AppModule.port = configService.get('HTTP_PORT')
}
}
async function bootstrap() {
const app = await NestFactory.create(AppModule);
await app.listen(AppModule.port || 3000);
}
bootstrap();
I have gone through official nestjs documentation. Here is what should be done in main.ts-
import { ConfigService } from '@nestjs/config';
const port = configService.get('PORT'); await app.listen(port|3000);
in .env PORT=3000
In app.module.ts , import config module if not done ;-) @Module({ imports: [ ConfigModule.forRoot({ isGlobal: true }), ] })
I'm submitting a...
Current behavior
Starting a new project includes an anti-pattern concerning the handling of configuration. Specifically,
src/main.ts
has the app listen on a hardcoded port.E.g.
On its own this is a seemingly small thing, but it elevates the risk of projects neglecting one of the tenants of The Twelve-Factor App:
https://12factor.net/config
A consumer of the framework would need to be aware of this anti-pattern and discover the documentation recommending how to properly handle configuration with Nest.
Expected behavior
Instead of the app listening to a hardcoded port, use a
ConfigService
class as suggested in the docs to pull the port from the environment.Documentation should also link users to the page on configuration techniques.
Minimal reproduction of the problem with instructions
Start a new project with Nest's CLI (
nest new project
) and see thatsrc/main.ts
has hardcoded port.What is the motivation / use case for changing the behavior?
Two motivations for this feature request:
Environment