Add support for using Winston in the browser

[joshacheson]

I'm opening this pull request now so that people can comment with ideas / criticisms etc.

Some things that could use input:

• What is considered proper detection for whether we're running in the browser as opposed to node? I've written a small helper function already. If anyone has ideas for better ways to detect then please let me know.
• We need to avoid use of the fs module in the browser. Immediately it occurs to me to wrap require calls to fs in try/catch and make generally to ensure that if winston is being run in the browser, fs reliant code is avoided altogether.
• Transports which rely inherently on fs or other node specific modules (perhaps http if there is no shim) should probably not be loaded. We should probably establish a pattern that avoids loading node specific modules altogether. Another option is to load these transports but throw errors if they're used in the browser.
• The current console transport wont work in browsers due to use of process.stderr/process.stdout. We could either load a different transport under the same name when running in the browser or provide a transport by a different (but similar) name such as browserConsole. I lean towards the former.

Please leave any comments / suggestions if you're inclined.

[joshacheson]

I'm thinking that perhaps the best route re: transports is to require a separate "transports" module altogether if running in the browser. This way we can leave the existing transports.js module as is, but use a different loading pattern altogether for browser transports.

[indexzero]

I like the idea, but I would prefer to see it fleshed out more. One small nitpick: filenames should always be dash-cased.js not camelCased.js.

[pose]

Hi @joshacheson, thanks for your PR!

My idea about this was that we should make it compatible with browserify. On that way, we don't need to handle the special cases with fs and we can use brfs shim. I've tried in the past to browserify this module but I've run into issues with the pkginfo module. I think that some transports won't be available but I don't think I'll be doing some explicit check like environmentIsBrowser. In the future, we plan to isolate winston core from the rest of the transports, so it would be easier to make it compatible with the browser. Thoughts?

[joshacheson]

@pose my concern with brfs (and i'll do the research to see what the deal is exactly) is that browserify is not the only CommonJS build system that gets a lot of use these days. Webpack is really popular (and becoming more so as time goes on) and I think using a solution that's only compatible with browserify would be a mistake.

The decision to isolate winston core from transports seems like a great one.

[pose]

@joshacheson Yes, I agree that making it only compatible with browserify would be a mistake. I'll need to do some research as well on how webpack implements CommonJS. Thanks!

[DiamondeX]

@indexzero, @pose, @joshacheson Two thoughts:

1. I think you must provide clear info on whether last stable version supports browser-side in package readme. And to provide separate subheader would be good idea. This will save time for many people.
2. I think that winston package must exclude any platform specific functionality from it's main entry point require hierarchy and let end-users to decide wich platform specific functionality (mostly transports) to include to their code along with winston core. This will reduce bundle size that browserify (or similar tools) produce.

Any feedback would be greatly appreciated

[DiamondeX]

Yet one thought:

1. Most browsers has their own mechanism to interpret arguments, provided to log methods of their native 'transport' - console. May be there is other transports with such trait. So logger must have way ('method' in mind) to distinguish such transport and pass non-auxiliary log arguments to the transport's log as is.

As before, any feedback would be greatly appreciated

[indexzero]

Going to close this out for now. If you'd like to continue pursuit of this feature please open a new PR. Thanks again for your contribution even though it didn't land.

[elyobo]

brfs is supported in webpack by https://github.com/webpack/transform-loader; perhaps this is worth reopening?

[dandv]

Looks like the main discussion is in #287