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

Logger #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Logger #46

wants to merge 3 commits into from

Conversation

evantahler
Copy link

It's generally useful for your web servers to have logs!

screen shot 2017-01-30 at 11 55 17 am

@evantahler
Copy link
Author

Closes #41

@evantahler evantahler mentioned this pull request Jan 30, 2017
@scottcorgan
Copy link
Owner

Can you elaborate just a bit on your choice of vocabulary used in the api (such as appenders)?

@evantahler
Copy link
Author

The vocabulary comes from https://github.com/nomiddlename/log4js-node. An appender is basically an output of the logger... it might be to the console (stdio), it might be to a file, it might be to some logging service. I'll bet for 90% of the use-cases for this server, logging to only console is OK.

Copy link
Owner

@scottcorgan scottcorgan left a comment

Choose a reason for hiding this comment

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

Just a few comments on the code.

Also, I like to take the safe approach and be explicit about default and custom configurations (from my Elm experience).

What do you think about the following APIs?

// Deafults to 'ALL' and 'console'
server.start({
  logger: true
});

// Allows you to configure to how you'd like?
server.start({
  customLogger: {
    level: 'All',
    appenders: [
      { type: 'console' }
    ]
  }
});

let onStarted = _onStarted || function () {}

app.use(compression())

// First, check the file system
// configure logger
Copy link
Owner

Choose a reason for hiding this comment

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

Should probably only use the logger if they provide a config for it (not default to all the time)

package.json Outdated
@@ -25,6 +25,7 @@
"compression": "^1.1.0",
"connect": "^3.2.0",
"connect-static-file": "^1.1.2",
"log4js": "^1.1.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Go ahead and lock the dependency here to the latest current version. (I locked the dependencies recently to avoid any unknown breaking changes)

@evantahler
Copy link
Author

I tend to prefer that logging is enabled by default... but OK!
Version locked and disabled by default :D

Copy link
Owner

@scottcorgan scottcorgan left a comment

Choose a reason for hiding this comment

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

Just a couple more changes. I have to be picky with this because it's used by some big projects and I don't want to break those projects.

Thanks!

{ type: 'console' }
]
},
});
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, just a couple more changes, then it's ready to merge! 🎉 . Should probably add an example here of using a logger default values.

var app = connect()

const PORT = 9000
const DIRECTORY = 'public'
const FILE = 'index.html'
const HOST = '0.0.0.0'
const LOGGEROPTIONS = {}
Copy link
Owner

Choose a reason for hiding this comment

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

I think the default value for this should be false. Then we could check if it's true or an object, then attach the logger.

@lukaspj
Copy link

lukaspj commented Sep 15, 2017

Any updates on this feature?

@scottcorgan
Copy link
Owner

@lukaspj can you move conversation to #41?

Thanks 👍

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.

3 participants