FoalTS / foal

Full-featured Node.js framework, with no complexity. 🚀 Simple and easy to use, TypeScript-based and well-documented.
https://foalts.org/
MIT License
1.89k stars 140 forks source link

@dependency creates multiple instances? #782

Closed crossinghoods closed 4 years ago

crossinghoods commented 4 years ago

I'm using FoalTS 1.11

Not sure if I'm getting the wrong idea, but I was under the impression that using either @depedency or ServiceManager().get(Service) would just use the same instance instead of creating multiple instances of the same service.

My debugging output

[1] Mail Service Instances 1 [1] GA Service Instances Created > 1 [1] Mail Service Instances 2 [1] GA Service Instances Created > 2 [1] Listening on port 3001... [1] Mail Service Instances 3 [1] GA Service Instances Created > 3

I've called GA Service using either @dependency or ServiceManager but yet it's called the constructor multiple times, same goes for the Mail Service. An instance is created for each controller that uses it the service; I've not tested it in other places such as models etc. but assume it will be the same

LoicPoullain commented 4 years ago

It shouldn't happen. Could you share your code?

crossinghoods commented 4 years ago

Hi LoicPoullain,

Thanks for the prompt response!

Here's a snippet of some of my code with the guts taken out, hope this helps.

some modifications to package.json "start:wi": "supervisor --inspect ./build -w ./build --no-restart-on error ./build/index.js", "develop": "npm run build:app && concurrently \"npm run build:app:w\" \"npm run start:wi\"",

ga.service.ts

export class GAService {
    @dependency cart: Cart;
    @dependency browser: Puppeteer;

    public static Instances = 0;

    constructor() {
        console.log("GA Service Instances Created >", ++GAService.Instances);
    }
}

mail.service.ts

import { Config } from "@foal/core";
import { EmailLog } from "../models";
import * as nodemailer from "nodemailer";

export class Mail {
    private transporter: any;

    constructor() {
        this.transporter = nodemailer.createTransport({
            host: Config.get("mail.smtp"),
            port: Config.get("mail.smtp-options.port"),
            secure: Config.get("mail.smtp-options.secure"), // true for 465, false for other ports
            auth: {
                user: Config.get("mail.smtp-options.user"), // generated ethereal user
                pass: Config.get("mail.smtp-options.password") // generated ethereal password
            }
        });
    }
}

automated-tasks.controller.ts

import {
    createService, dependency, ServiceManager
} from "@foal/core";
import { MongoDBStore } from "@foal/mongodb";
import { Mail, GatewayService } from "../services";

export class AutomatedTasksController {
    @dependency public mail: Mail;
    @dependency public gateway: GatewayService;

    constructor() {
        const sm = new ServiceManager();
        if (!this.mail) {
            this.mail = sm.get(Mail);
        }

        if (!this.gateway) {
            this.gateway = sm.get(GatewayService);
        }

    }
}

another.controller.ts

@JWTRequired()
export class AnotherController {
    @dependency public gateway: GatewayService;
    @dependency public mail: Mail;

   @Get("/orders")
    public async test(ctx: Context){
return new HttpResponseOK("test");
}
}
LoicPoullain commented 4 years ago

This is normal. In AutomatedTasksController, you create a new ServiceManager which is different from the built-in one used by the framework. This is why some services are instantiated twice. Why not removing the constructor content? The properties mail and gateway should already be set.

crossinghoods commented 4 years ago

Hi LoicPoullain,

I want to have my system run some tasks automatically instead of using a external Crontab, if you can direct me on how to do that instead it would be great! 👍

My current app.controller.ts

export class AppController {
 constructor() {
        // Kick off scheduled tasks after 2 second of launching
        setTimeout(() => {
            this.scheduledTasks();
        }, 2000);
    }

private scheduledTasks(): void {
        const taskManager = new AutomatedTasksController();
        try {
            // Send emails every min
            this.cron.addTask("processEmails", "* * * * *", () => taskManager.processEmails());
}catch (e) {
            console.error("Scheduled Tasks Error: ", e);
        }
}
}
crossinghoods commented 4 years ago

Think I figured it out, so instead of creating a Service Manager instance myself I just use dependency to get an instance of it. in the controller. I have now gotten rid of the constructor in AutomatedTasksController so it seems to only be creating 1 instance of the service 👍 Feel free to mark this as resolved

export class AppController {
**@dependency private taskMan: AutomatedTasksController;**

private scheduledTasks(): void {
        const taskManager = new AutomatedTasksController();
        try {
            // Send emails every min
            this.cron.addTask("processEmails", "* * * * *", () => this.taskMan.processEmails());
         }catch (e) {
            console.error("Scheduled Tasks Error: ", e);
        }
}
}