strip console.* in dist/flow.min.js

[drzraf]

Addressing a comment from #304

I suggest either:

• to only drop console.* from one of the bundle (.js or .min.js) so that the other can be used during test (and still show the output)
• either do it based on an environment variable.

Keeping console.log accelerates development and it sounds better to leave to the toolchain the job of striping them when needed.

[AidasK]

I don't think adding console.log to the lib is a good practice. It just means that code is clunky and it needs to be refactored instead.

[drzraf]

Many software, even those not involving complex chunking and concurrency heuristic, end-up with some logging facilities (including a debugging level). Even a low-level graphic library like GDK for Gnome offer advanced debuging via environment variable, available in their -production build on the end-user desktop.

Indeed we could as well write a FlowChunk::_log: function(str, args = []) and other toString methods to convince ourselves the logging code is well limited, encapsulated and/or only activated when needed, but in the end this is just reinventing console.log + rollup strip().

The toolchain offers a chance of creating both good -production and very convenient -debug builds.
Would a FLOW_DEBUG environment variable be an acceptable compromise? (If it's not present both (.js / .min.js) builds would be stripped of their console.log)?

[AidasK]

Maybe more versatile solution would be to add Flow.log function. E.g.:

log: function (message) {
if (this.opts.log) {
console.log(message);
}
}

And usage would be:
this.flowObj.log('')

Though it should be disabled by default.

[drzraf]

But don't you think that all occurrences of console.log (or flowObj.log) must be strip()ped  (unless it's a debug build) using a compile-time environment variable?

[AidasK]

I don't think they need to be stripped if they are disabled by a variable by default. Package size is going to be the same anyway.

[drzraf]

@evilaliv3 : Any opinion?

[AidasK]

I gave a thought about these logs during my sleep and I have an elegant idea. Why not to make it an event? this.fire('log', message). And developers can hook them if they want to debug it. Simple as that.

flow.on('log', (m) => console.log(m))

[drzraf]

Why not to make it an event? this.fire('log', message). And developers can hook them if they want to debug it. Simple as that.

Pro:

1. It's available for everyone at runtime
2. It can be enabled/disable dynamically at runtime even for -production builds
3. No new function nor significant namespace problem

Cons:

• Can't be used within fire itself without the risk of triggering infinite loops (and use within other event handler could have side effects).
• Contrary to a straight function call, it can't be stripped automatically from a build by toolchain
• It's generic, but would probably have very little use other than console.* calls anyway.

Personally I still like the idea of just stripping console.log in dist/flow.js and dist/flow.js but just adding a dist/flow.dev.js build target (git-ignored / non-versioned). Switching debug to "on" is as easy as <script src="dist/flow.js"> and developers start seeing all the handy debug message.

I previously gave an argument that production software builds containing debugging facility actionable at runtime.
But the only thing actually interesting part is the availability of console.log in the codebase without having to add them every time we develop then removing them for every PR.

[AidasK]

I must disagree. Striping console.log from the build is harder than just remove it from the hook.

From my point of view, console log messages should be left with deprecation messages and errors. These are a serious messages and they will never appear in the console once fixed. This this pull request we are going to remove these messages as well, which I would like to avoid.

Every other informational/debug console.log should be done via flow.fire().

Other libraries does that too and this way my console stays clean. I only want to see these messages if I am debugging this library. After it's implemented I want to focus on the other stuff and these messages does nothing more than distract.

[drzraf]

From my point of view, console log messages should be left with deprecation messages and errors. These are a serious messages and they will never appear in the console once fixed. This this pull request we are going to remove these messages as well, which I would like to avoid.

In the particular case of this PR (which was mostly to discuss the idea rather than a perfect implementation), it would have only stripped console.(log|debug), but kept console.(info|warn|error) => Giving the ability to distinguish between developer-intended and long-term/important user-intended messages. (With developer-intended messages still being thrown on a compile-time opt-in basis)

[AidasK]

let's keep this going, better something than nothing. You can merge this