Windows log fixes #1307

XhmikosR · 2016-12-25T09:57:21Z

This is #1272 cherry picked, #1305 cherry picked and fixed.

Verified everything works on my Windows VM.

Closes #1272, closes #1305.
Fixes #1228, fixes #1261.

Note to maintainers: Do not squash the branch when merging in order to keep author information.

Oh, and merry Christmas 🎄

/CC @Janpot to confirm you are OK with cherry picking your patch and close 1272.

googlebot · 2016-12-25T09:57:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

AdrianoCahete · 2016-12-25T18:18:49Z

The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

For me, it's ok.

Janpot · 2016-12-26T14:04:03Z

Not sure I agree with the proposed solution for windows. This seems to be a problem with cmd.exe not supporting unicode by default. I guess this change would affect anybody on windows, regardless of which shell they use.
Nevertheless I'm in favour of pragmatic solutions so if the owners agree with this one, it's ok for me.
If you go with this one however, I think it should be abstracted away like the figures module does. Maybe even contribute to them some of the substitutions that are missing for the tree?

XhmikosR · 2016-12-26T14:59:48Z

In my original patch I used figures. But being that here they don't use chalk either, I just kept the original patch @AdrianoCahete had made.

I'm not sure what you mean by abstracted. If you mean have the common code in one place, that would make sense. On the other hand, I don't see why should reinvent the wheel and not just use figures and chalk for colors.

Anyway, the fact is that this works with the default shell Windows comes with and thus fixes the issue I reported.

XhmikosR · 2016-12-26T15:06:38Z

@Janpot: Also, the CC here was for you to confirm you allow the use of your patch, which you didn't.

wardpeet · 2016-12-26T17:12:10Z

I have to agree with @Janpot that abstraction would be a better way to deal with this.
Including figures will add an extra dependency that we might not want/need (@paulirish)
we could add a simple abstraction file ourselves with just icons based on runtime.

The fix works in cmd & powershell but not cygwin

Janpot · 2016-12-26T18:07:45Z

Yep, I mentioned figures just as an illustration of my reasoning. Whether you want an extra dependency or not is author's preference to me. In my own code I reuse as much as possible but I acknowledge there's a cost to that as well. For me the important thing is not to litter main business logic with platform switches.
I'd also keep both PRs separate as my contribution merely deals with the latest update of the debug package. The unicode stuff is part of a broader issue I guess.
Also want to stress this is not an ego thing or anything, if lighthouse authors want to merge this PR they totally have my ok, even if if they squash (I care about good software, not about bragging rights), just want to make sure I stated my point of view before they do so.

wardpeet · 2016-12-26T19:30:08Z

I would wait for @paulirish, @patrickhulce or @brendankenny to give the final judgement :)

@XhmikosR Thanks for this pr but as janpot says it are 2 seperates issues. You should fix unicode issues in this PR :)
@Janpot don't worry about it ;) it's fine.

AdrianoCahete · 2016-12-27T01:08:10Z

The fix works in cmd & powershell but not cygwin

Cygwin seems to don't show unicode by default.
I never used Cygwin, I should probably be wrong.

ebidel · 2016-12-30T21:07:58Z

Since the other committers are ok with the cherry pick, I think we can move forward with this one. I'm going to close #1272 to focus efforts here.

In the future, we'd prefer if individual PRs fix individual issues. This one is getting a bit hard to follow because of the cherry picking and multiple issues that are being referenced.

ebidel · 2016-12-30T21:10:56Z

lighthouse-cli/chrome-launcher.ts

@@ -191,7 +191,11 @@ class ChromeLauncher {
        launcher
          .isDebuggerReady()
          .then(() => {
-            log.log('ChromeLauncher', waitStatus + `${green}✓${reset}`);
+            if (process.platform === 'win32') {


how about:

// See https://github.com/GoogleChrome/lighthouse/issues/1228 const check = process.platform === 'win32' ? '\u221A' : '✓'; log.log('ChromeLauncher', waitStatus + `${green}${check}${reset}`);`

Sounds good to me, I'll change it to that.

ebidel · 2016-12-30T21:11:21Z

lighthouse-cli/printer.ts

@@ -56,8 +56,8 @@ function formatAggregationResultItem(score: boolean | number | string, suffix?:
  const reset = '\x1B[0m';

  if (typeof score === 'boolean') {
-    const check = `${green}✓${reset}`;
-    const fail = `${red}✘${reset}`;
+    const check = process.platform === 'win32' ? `${green}\u221A${reset}` : `${green}✓${reset}`;


similar suggestion

ebidel · 2016-12-30T21:11:50Z

lighthouse-cli/printer.ts

@@ -97,7 +97,11 @@ function createOutput(results: Results, outputMode: OutputMode): string {
  let output = `\n\n${bold}Lighthouse (${version}) results:${reset} ${results.url}\n\n`;

  results.aggregations.forEach(aggregation => {
-    output += `▫ ${bold}${aggregation.name}${reset}\n\n`;
+    if (process.platform === 'win32') {


similar suggestion to dry this up.

ebidel · 2016-12-30T21:12:12Z

lighthouse-core/formatters/critical-request-chains.js

@@ -148,7 +148,11 @@ class CriticalRequestChains extends Formatter {

        // If the parent is the last child then don't drop the vertical bar.
        const ancestorTreeMarker = treeMarkers.reduce((markers, marker) => {
-          return markers + (marker ? '┃ ' : '  ');
+          if (process.platform === 'win32') {
+            return markers + (marker ? '\u2502 ' : '  ');


XhmikosR · 2016-12-31T17:52:20Z

@wardpeet: I don't use cygwin so I can't test there. MSYS works fine on my test VM.
@ebidel: simplified code per your comments.

Now some things to note:

There's code in tests that uses symbols and should also be taken care of.
Due to 1) it might make more sense to abstract the common parts to a separate file to include in the whole codebase.
If a 3rd-party lib does the job, perhaps we could use that. Oh, and see if we can make the blue lighter so that it shows better on Windows.

ebidel

Few more small things.

ebidel · 2016-12-31T18:20:50Z

lighthouse-core/formatters/critical-request-chains.js

-          return markers + (marker ? '┃ ' : '  ');
+          // See https://github.com/GoogleChrome/lighthouse/issues/1228
+          const sym = process.platform === 'win32' ? '\u2502 ' : '┃ ';
+            return markers + (marker ? sym : '  ');


indentation is funky here.

ebidel · 2016-12-31T18:21:50Z

lighthouse-core/formatters/critical-request-chains.js

+        let treeMarker = ancestorTreeMarker;
+        if (process.platform === 'win32') {
+          treeMarker += (isLastChild ? '\u2514\u2500' : '\u251C\u2500') +
+            (hasChildren ? '\u252C' : '\u2500');


4 space indent

Should it be 4 space indentation? I don't see this being enforced by ESLint ;/

Correct. We do it elsewhere, but sadly, I don't think there's a rule.

This seems fine as a 2-space indentation. I added the rule locally to ESLint and it's passing.

This seems fine as a 2-space indentation

definitely needs a four space indent after a continuation like this :)

Happy to have a rule to enforce that...may just be the eslint config didn't bring that over when we updated it recently.

ebidel · 2016-12-31T18:21:55Z

lighthouse-core/formatters/critical-request-chains.js

+          treeMarker += (isLastChild ? '\u2514\u2500' : '\u251C\u2500') +
+            (hasChildren ? '\u252C' : '\u2500');
+        } else {
+          treeMarker += (isLastChild ? '┗━' : '┣━') +
            (hasChildren ? '┳' : '━');


4 space indent

ebidel · 2016-12-31T18:22:48Z

Can you also look into the failing tests?

XhmikosR · 2016-12-31T18:28:47Z

@ebidel: the tests failure is due to the expired SSL certificate

shop.polymer-project.org uses an invalid security certificate. The certificate expired on Σάββατο, 31 Δεκεμβρίου 2016 02:00. The current time is Σάββατο, 31 Δεκεμβρίου 2016 20:28. Error code: SEC_ERROR_EXPIRED_CERTIFICATE

ebidel · 2016-12-31T18:33:21Z

Derp, thanks for the heads up on the ssl cert. I'll tell the Polymer team.

Can you file an issue for the remaining items?

XhmikosR · 2016-12-31T18:34:22Z

Which issues? We mentioned many things here :p

ebidel · 2016-12-31T18:45:20Z

#1307 (comment) and anything else this PR doesn't currently cover :)

XhmikosR · 2016-12-31T18:46:12Z

All right, I'll try to wrap everything up and make a couple of issues next year :)

brendankenny · 2016-12-31T18:43:13Z

lighthouse-cli/chrome-launcher.ts

@@ -191,7 +191,9 @@ class ChromeLauncher {
        launcher
          .isDebuggerReady()
          .then(() => {
-            log.log('ChromeLauncher', waitStatus + `${green}✓${reset}`);
+            // See https://github.com/GoogleChrome/lighthouse/issues/1228
+            const check = process.platform === 'win32' ? '\u221A' : '✓';


can you pull this into a file-level global a la green and reset?

You mean along with green and reset at the top of the file?

brendankenny · 2016-12-31T18:44:30Z

lighthouse-cli/printer.ts

-    const check = `${green}✓${reset}`;
-    const fail = `${red}✘${reset}`;
+    // See https://github.com/GoogleChrome/lighthouse/issues/1228
+    const check = process.platform === 'win32' ? `${green}\u221A${reset}` : `${green}✓${reset}`;


globals here too

brendankenny · 2016-12-31T18:46:00Z

lighthouse-core/formatters/critical-request-chains.js

-            (hasChildren ? '┳' : '━');
+        let treeMarker = ancestorTreeMarker;
+        if (process.platform === 'win32') {
+          treeMarker += (isLastChild ? '\u2514\u2500' : '\u251C\u2500') +


maybe do this as with ancestorTreeMarker above, where the symbol is picked conditionally (maybe at file level) but all platforms construct the string in the same way?

I don't mind doing this, I"m just having trouble naming those symbols :/

brendankenny · 2016-12-31T18:46:59Z

lighthouse-core/formatters/critical-request-chains.js

@@ -148,7 +148,9 @@ class CriticalRequestChains extends Formatter {

        // If the parent is the last child then don't drop the vertical bar.
        const ancestorTreeMarker = treeMarkers.reduce((markers, marker) => {
-          return markers + (marker ? '┃ ' : '  ');
+          // See https://github.com/GoogleChrome/lighthouse/issues/1228
+          const sym = process.platform === 'win32' ? '\u2502 ' : '┃ ';


file-level consts for these may work better, too.

brendankenny · 2016-12-31T18:47:46Z

lighthouse-core/lib/log.js

-  const debugNode = require('debug/node');
-  debugNode.colors = [colors.cyan, colors.green, colors.blue, colors.magenta];
-}
+debug.colors = [colors.cyan, colors.green, colors.blue, colors.magenta];


how does this play along with the changes @paulirish was making in #915? We don't want to regress the browserified build

another option is to lock the package.json version to debug 2.2.0 as our yarn.lock file does

This PR shouldn't affect anything else other than Windows console.

yeah this simplification shoulda been in 915. lgtm

ebidel · 2016-12-31T18:57:53Z

Confirming the smokehouse tests pass if you remove shop.

XhmikosR · 2017-01-01T08:14:35Z

@brendankenny @ebidel: please check now and let me know.

Ah, I found where I saw this https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/smokehouse/smokehouse.js#L36

So, ideally, after this PR goes in, all common related stuff should go into the same file to prevent WET.

paulirish · 2017-01-03T22:05:32Z

lighthouse-core/lib/log.js

-  const debugNode = require('debug/node');
-  debugNode.colors = [colors.cyan, colors.green, colors.blue, colors.magenta];
-}
+debug.colors = [colors.cyan, colors.green, colors.blue, colors.magenta];


yeah this simplification shoulda been in 915. lgtm

paulirish · 2017-01-03T22:11:25Z

lighthouse-cli/chrome-launcher.ts

+const reset = '\x1B[0m';
+
+// See https://github.com/GoogleChrome/lighthouse/issues/1228
+const tick = process.platform === 'win32' ? '\u221A' : '✓';


Can we simplify and just always use the unicode escape, and never the literal?

const tick = '\u221A'; // ✓

(I'm assuming the windows character in play is the exact same as the literal... that right?)

looks like they're different. ✓ vs √

okay. then we'll stick with this hilarity.

Let's pull process.platform === 'win32' out into a isWindows boolean then
@XhmikosR , sg?

Yeah, unfortunately, the normal tick is bold and looks a lot better on normal terminals, i.e. not Windows :/

I'll add the const now and this can be refactored later, see #1307 (comment)

Janpot · 2017-01-03T23:25:03Z

@paulirish just want to point out it's not just a simplification. debug changed their directory structure and require('debug/browser') simply doesn't resolve to any file anymore.

browserify selects correct file automatically. Also `log.color = undefined` causes problems.

paulirish · 2017-01-04T18:27:44Z

Seriously great group effort on this. Thanks all!

XhmikosR mentioned this pull request Dec 25, 2016

Fix for Windows Environments #1305

Closed

ebidel added the windows label Dec 30, 2016

ebidel mentioned this pull request Dec 30, 2016

Fix logging #1272

Closed

ebidel suggested changes Dec 30, 2016

View reviewed changes

ebidel suggested changes Dec 31, 2016

View reviewed changes

brendankenny requested changes Dec 31, 2016

View reviewed changes

ebidel approved these changes Jan 3, 2017

View reviewed changes

ebidel assigned brendankenny Jan 3, 2017

paulirish requested changes Jan 3, 2017

View reviewed changes

Fix after upstream changes to debug.

8c13f5f

browserify selects correct file automatically. Also `log.color = undefined` causes problems.

AdrianoCahete and others added 2 commits January 4, 2017 09:52

Fix for tick/cross symbols on Windows.

2f30a25

Fix lint issues and simplify things.

7e2ff0c

paulirish approved these changes Jan 4, 2017

View reviewed changes

paulirish merged commit 0fd4753 into GoogleChrome:master Jan 4, 2017

XhmikosR deleted the windows-log-fixes branch January 4, 2017 18:27

XhmikosR mentioned this pull request Jan 5, 2017

Update ESLint config to check for indentation too #1415

Closed

Windows log fixes #1307

Windows log fixes #1307

Conversation

XhmikosR commented Dec 25, 2016 • edited Loading

googlebot commented Dec 25, 2016

AdrianoCahete commented Dec 25, 2016

Janpot commented Dec 26, 2016 • edited Loading

XhmikosR commented Dec 26, 2016

XhmikosR commented Dec 26, 2016

wardpeet commented Dec 26, 2016

Janpot commented Dec 26, 2016

wardpeet commented Dec 26, 2016

AdrianoCahete commented Dec 27, 2016

ebidel commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XhmikosR commented Dec 31, 2016

ebidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Dec 31, 2016

XhmikosR commented Dec 31, 2016

ebidel commented Dec 31, 2016

XhmikosR commented Dec 31, 2016

ebidel commented Dec 31, 2016

XhmikosR commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Dec 31, 2016

XhmikosR commented Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot commented Jan 3, 2017

paulirish commented Jan 4, 2017

XhmikosR commented Dec 25, 2016 •

edited

Loading

Janpot commented Dec 26, 2016 •

edited

Loading

XhmikosR commented Jan 1, 2017 •

edited

Loading