Allow global headless setting #8037
Conversation
Any way we can confirm this? Would be big news if true. |
Sadly I could not run this against my bigger project from checkout because my project uses newer version of Blaze and I could not convince Meteor from checkout to use that version of Blaze. So I was unable to measure on a large codebase to have significant results. |
|
I've experienced this in the Qualia application as well (with progress updates from the build tool in general, not just specifically the spinner), although it's a little difficult to reproduce. The fundamental issue is that Node's logging statements are synchronous and the progress updates sends many thousands of messages to the console. So if the log buffer gets filled up everything slows to a crawwwwwwllllllll. I've tried commenting out the logging and it definitely speeds things up. Here is a representative sample from a CPU profile of our application during the initial portion of the build tool where it is printing out a bunch of stuff:
This CPU profile is from Meteor 1.3, not Meteor 1.4, but I don't see why that would've changed this problem. On this reload, it spent 1.9s (of the first 3 seconds of a reload) in |
|
While performance is important and ultimately, yes, this could be disabled, I disagree that there is "No need for a spinner at all". I feel that losing the spinner for the whole of the As @veered mentioned above, the blocking nature of Also, I haven't looked at the console code but if the spinner was moved to the beginning of the line, maybe the amount of output could be substantially reduced since it would be possible to overwrite only the spinner portion instead of the whole message (i.e. fewer bytes). |
Really? Now that things are running so fast, I think there is always some message quickly flickering between my eyes. Anyway, let's merge this. And then others can measure performance and also we can later on decide if to remove/change spinner at all. |
I prefer that all technology show its user that it is doing something versus nothing at all. :) But yes, if it just sat there saying
Agree, the above convo/profile should be broken into a separate issue for discussion. |
| @@ -309,7 +309,7 @@ var ProgressDisplayFull = function (console) { | |||
| self._progressBarRenderer = new ProgressBarRenderer(PROGRESS_BAR_FORMAT, options); | |||
| self._progressBarRenderer.start = new Date(); | |||
|
|
|||
| self._headless = false; | |||
| self._headless = !!(process.env.METEOR_HEADLESS && process.env.METEOR_HEADLESS != '0'); | |||
There was a problem hiding this comment.
Is the != '0' necessary? If so, it should probably use JSON.parse in the same manner as METEOR_ALLOW_SUPERUSER, but really, why not just have it work like most other env variables where if it's set in the environment, it's true otherwise, false: !!(process.env.METEOR_HEADLESS)
Either way, it should use strict equality !== not !=.
There was a problem hiding this comment.
I reused the same approach from this same file: https://github.com/mitar/meteor/blob/3ee60f57e57b56b56b36e8c8cdbdbb79c9b55e25/tools/console/console.js#L74
I wanted to be consistent inside a file.
There was a problem hiding this comment.
I appreciate the consistency, but I was just about to link to the same METEOR_ALLOW_SUPERUSER example that @abernix mentioned above, because I think it's important to support e.g. METEOR_HEADLESS=false.
| @@ -309,7 +309,7 @@ var ProgressDisplayFull = function (console) { | |||
| self._progressBarRenderer = new ProgressBarRenderer(PROGRESS_BAR_FORMAT, options); | |||
| self._progressBarRenderer.start = new Date(); | |||
|
|
|||
| self._headless = false; | |||
| self._headless = !!(process.env.METEOR_HEADLESS && process.env.METEOR_HEADLESS != '0'); | |||
There was a problem hiding this comment.
I appreciate the consistency, but I was just about to link to the same METEOR_ALLOW_SUPERUSER example that @abernix mentioned above, because I think it's important to support e.g. METEOR_HEADLESS=false.
|
Updated. |
This should also fix a bug where Console.isHeadless() was returning false in tests. Follow-up to #8037.
|
@veered: Have you recently measures how quick is building time if you disable output now? |
|
I haven't upgraded our version of Meteor yet; I'll let you know when I do & maybe post a profile |
This is a followup to #7814. There are simply many reasons why would various commands be run inside CI and other automatic environments. Spinner is not needed there. Instead of adding
--headlessarguments to various commands, let's have a global environment variable and this is it.BTW, this also seems to improve speed of whole Meteor tool experience. I would even say that probably this should be the default. No need for a spinner at all. It slows down things too much.