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

Support for Boolean Type in Logger Option #24

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

15fathoms
Copy link
Contributor

Description

This pull request adds support for using a boolean value in the logger option within the Options interface. This enhancement allows users to enable or disable logging functionality with a simple boolean flag (true or false) in addition to the existing capability of providing a custom logging function.

Detailed Changes

  1. Modified Options Interface:

  • The logger property in the Options interface now accepts either a function of type (method: string, space: string, path: string) => void or a boolean.
  1. Updated expressListRoutes Function:

  • Implemented logic to handle logger being true, false, or a function.
  • When logger is true, the default logger console.info is used.
  • When logger is false, no logging occurs.
  • When logger is a function, the provided function is used for logging.

Benefits

  • Flexibility: Users can now easily enable or disable logging with a boolean flag.
  • Backward Compatibility: Existing functionality with a custom logging function remains unchanged.
  • Simplicity: Simplifies the configuration for users who only need basic logging enabled or disabled.

Example Usage

const expressListRoutes = require('express-list-routes');

//  logger enabled, using default logger (true)
expressListRoutes(app, {
  logger: true,
});

// logger disabled (false)
expressListRoutes(app, {
  logger: false,
});

// custom logger
expressListRoutes(app, {
  logger: console.warn
});

// default
expressListRoutes(app);

XOXO ❤️

@labithiotis
Copy link
Owner

@15fathoms Thank you for taking time to make a PR and writing clear PR description.

I have one question/observation, can users achieve the same result by not calling library?

For example:

if (!CONFIG.DISABLE_LOGGING) {
  expressListRoutes(app, { prefix: '/api/v1' });
}

vs

expressListRoutes(app, { prefix: '/api/v1', logging: CONFIG.DISABLE_LOGGING });

I'm not totally against merging the change but I would urge users to do the former solution because it will avoid running code all together which may effect performance boot time if you have lots of routes.

@15fathoms
Copy link
Contributor Author

@labithiotis

Thank you for your question. I realize now that I may have omitted the most important detail of this pull request. Personally, the aspect of your module that interests me is the returning of routes. I use it to sort some of the routes and the data that results from them.

Although my project is not yet public, I want to illustrate my point. Here is an example of how I use express-list-routes:

const endpoints = expressListRoutes(req.app, {
    logger: false
});
endpoints.forEach((endpoint) => {
    endpoint.path = endpoint.path.split('\\').join('/');
    if (endpoint.path.endsWith('/')) {
        endpoint.path = endpoint.path.slice(0, -1);
    }
});
const filteredEndpoints = endpoints.filter((endpoint) => {
    if (endpoint.method.includes('GET')) {
        return collectionNames.some((collection) => endpoint.path.endsWith(collection));
    }
    return false;
});

It is possible to conditionally call the function and use dynamic import to load the module only when the condition is met and passing logger as false.

Looking at your code, we can see that, whether logs are enabled or not, the loop is still executed. Including an empty function doesn't help with performance either.

To improve performance, it might be interesting to separate the logic into multiple functions and call them either by default or based on defined options. However, looping is still necessary.

My pull request is a request for long-term support for this feature for two reasons:

  1. My project depends on this module, and during deployment and installation of node modules, my code is no longer functional, which would be problematic.

  2. I could eventually publish my own package, but since this one already meets my needs (and certainly those of many other developers), I don't see the point of creating redundancy for a simple feature and also duplicating your work.

@labithiotis
Copy link
Owner

@15fathoms Ahh, it never occurred to me that returning a list of routes would be useful. But I now see how helpful it can be to have a pragmatic list of all your routes registered, especially in larger apps where app is piped down to nested folders/projects.

Again thank you for taking the time to improve this PR. I will merge this PR as is, I will also tweak docs to highlight that use case.

Btw, the only reason I returned paths was for testing, this was a happy side effect of doing that 😂

@labithiotis labithiotis merged commit 8e0227d into labithiotis:main Jul 7, 2024
1 check passed
@labithiotis
Copy link
Owner

Version 1.2.2 is released https://www.npmjs.com/package/express-list-routes?activeTab=versions

@15fathoms
Copy link
Contributor Author

@labithiotis Thanks fellow dev friend ! 😁

Well, actually the happy side effect is REALLY useful ! For exemple, here's what i can return to list my routes inside my collections route :

Given the URL http://localhost:3000/api/collections :

[
  {
    "name": "fonts",
    "model": "Font",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/fonts"
    }
  },
  {
    "name": "news",
    "model": "New",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/news"
    }
  },
  {
    "name": "models",
    "model": "Model",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/models"
    }
  },
  {
    "name": "images",
    "model": "Image",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/uploads"
    }
  },
  {
    "name": "sections",
    "model": "Section",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/sections"
    }
  },
  {
    "name": "users",
    "model": "User",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/users"
    }
  },
  {
    "name": "galleries",
    "model": "Gallery",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/galleries"
    }
  },
  {
    "name": "configs",
    "model": "Config",
    "endpoint": {
      "method": [
        "GET",
        "POST"
      ],
      "path": "/api/configs"
    }
  }
]

Or given the exemple url : http://localhost:3000/api/collections/search?collection=Font&searchQuery=DM%20Mo

[
  {
    "_id": "667a0e01d894fcd15bf968fe",
    "family": "DM Mono",
    "variants": [
      "300",
      "300italic",
      "regular",
      "italic",
      "500",
      "500italic"
    ],
    "subsets": [
      "latin",
      "latin-ext"
    ],
    "__v": 0
  }
]

As you can see, it actually can help dynamically doing a lot off stuff depending on what you're working on.
Do you mind if i keep working on this module to enhance it ?

Thank you a lot for your time. 😊

@labithiotis
Copy link
Owner

@15fathoms your more than welcome to submit PRS, I will always try to respond in a timely manner.

😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants