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

New DebugLogger #1302

Merged
merged 10 commits into from
Dec 6, 2017
Merged

New DebugLogger #1302

merged 10 commits into from
Dec 6, 2017

Conversation

pravdomil
Copy link
Contributor

@pravdomil pravdomil commented Dec 5, 2017

Motivation

The debug package is the 9th most dependent package, it's used by express, koa, yeoman, babel, mocha, socket.io, eslint, morgan and more. It works in both node.js and browser.
#634 #633

What's new

  • ConnectOptions.logger has new value "debug"
  • New debug namespaces introduced:
typeorm:query
typeorm:queryError
typeorm:querySlow
typeorm:schemaBuild
typeorm:migration
typeorm:log
typeorm:info
typeorm:warn

Drawbacks

  • Debug dependency (debug itself has only one dependency, module ms)
  • ConnectOptions.logging?: LoggerOptions is no longer used.
    You need set env variable DEBUG=typeorm:* or call debug.enable('typeorm:*'); in order to set log level.

Benefits

  • TypeORM log fits nicely next to others.
  • You can start debugging every aspect of application by setting DEBUG=* and logger: "debug".

Example

Here's rest api example, I got a get request and then it reports which query TypeORM executed.

api

Discussion

  • Use it as default logger and deprecate logging option and control everything via debug lib. That's basically how express does logging.
  • Documentation changes

@@ -0,0 +1,103 @@
import * as debug from "debug";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to dynamically load any third-party package using require from PlatformTools class. Its requirement for typeorm to be cross-platform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import * as path from "path";

/**
 * Platform-specific tools.
 */
export class PlatformTools {
    ...
		
    /**
     * Debug module needed by DebugLogger
     */
    static debug = debug;
}

like that? what do you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, just see how many other libraries like mkdirp, redis, mysql, etc. are loaded using PlatformTools, its dynamic loading using require function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh you meant PlatformTools.load

private debugInfo = debug("typeorm:info");
private debugWarn = debug("typeorm:warn");

constructor() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it

*/
export class DebugLogger implements Logger {
private debugQuery = debug("typeorm:query");
private debugQueryError = debug("typeorm:queryError");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeorm:queryError

queryError looks rough, is there naming convention in debug? Maybe better name it typeorm:query:error or typeorm:query-error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pleerock
Copy link
Member

pleerock commented Dec 5, 2017

Thank you for your contribution, can you please update documentation and changelog for the next version as well

@pravdomil
Copy link
Contributor Author

what is the next version number?

@pleerock
Copy link
Member

pleerock commented Dec 5, 2017

0.1.8

@pravdomil
Copy link
Contributor Author

pravdomil commented Dec 5, 2017

maybe we can change typeorm:query* debug namespace to

typeorm:query:log
typeorm:query:error
typeorm:query:slow

and

typeorm:schemaBuild to typeorm:schema

@pleerock pleerock merged commit 9c55f29 into typeorm:master Dec 6, 2017
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.

None yet

2 participants