Fix xo 0.17 errors and use root xo config for app #859

ekmartin · 2016-10-11T16:02:19Z

I've ignored import/prefer-default-export - as we have a lot of plural filenames that currently only export one function (basically most of ./lib/utils). We could change these to default exports, but I don't think it makes it any clearer (rather the opposite).

ekmartin · 2016-10-11T16:02:59Z

app/package.json

@@ -9,6 +9,7 @@
  },
  "repository": "zeit/hyper",
  "description": "HTML/JS/CSS Terminal",
+  "xo": false,


This makes xo look upwards in the directory hierarchy for the next package.json, see xojs/xo#151.

I would recommend placing non-standard package.json properties at the bottom.

ekmartin · 2016-10-11T16:03:06Z

lib/actions/sessions.js

@@ -88,7 +88,7 @@ function createExitAction(type) {
        }

        const sessions = keys(getState().sessions.sessions);
-        if (!sessions.length) {
+        if (sessions.length === 0) {


Readable and consistent code ;)

sessions.length === 0 sessions.length > 3 sessions.length === 5 !sessions.length 👺

Still, I would usually write if (!something) instead of if (something === false) - even if I only wanted to compare it to false (not null etc.). If it was up to me an empty array would be falsey as well, and I'd have just written if (!sessions).

!something and !something.length is not the same thing. The first one is a lot clearer than the latter.

ekmartin · 2016-10-11T16:04:16Z

lib/components/term.js

-          onMouseEnter={this.handleScrollEnter}
-          onMouseLeave={this.handleScrollLeave}
-          />
+            <div


I had to indent the second part of the ternary here, not sure if this is intended behaviour or a bug. @sindresorhus?

What was the error?

Hmm, maybe related to jsx-eslint/eslint-plugin-react#901

That being said, the code there is not very readable. I would personally have put them in two separate variables and used the variables in the ternary instead.

Ye, I would've probably used a couple of extra stateless components or a render function as well - should take a look at it some time. Thanks for the link.

sindresorhus · 2016-10-11T17:28:17Z

app/package.json

@@ -9,6 +9,7 @@
  },
  "repository": "zeit/hyper",
  "description": "HTML/JS/CSS Terminal",
+  "xo": false,


I would recommend placing non-standard package.json properties at the bottom.

sindresorhus · 2016-10-11T17:28:57Z

app/plugins.js

@@ -1,3 +1,4 @@
+/* eslint-disable import/no-dynamic-require */


This would be better placed inline with the code it applies to. eslint-disable-line can be used or eslint-disable-next-line.

Changed it. Didn't know about eslint-disable-next-line - thanks.

sindresorhus · 2016-10-11T17:32:15Z

lib/actions/sessions.js

@@ -88,7 +88,7 @@ function createExitAction(type) {
        }

        const sessions = keys(getState().sessions.sessions);
-        if (!sessions.length) {
+        if (sessions.length === 0) {


Readable and consistent code ;)

sessions.length === 0 sessions.length > 3 sessions.length === 5 !sessions.length 👺

sindresorhus · 2016-10-11T17:32:45Z

lib/components/term.js

-          onMouseEnter={this.handleScrollEnter}
-          onMouseLeave={this.handleScrollLeave}
-          />
+            <div


What was the error?

sindresorhus · 2016-10-11T17:33:09Z

package.json

      "no-warning-comments": 0,
      "complexity": 0,
      "no-nested-ternary": 0,
+      "import/no-extraneous-dependencies": 0,


You can remove this. Not needed anymore as XO handles electron.

Thanks, found out that we missed another dependency in app as well... 👍

Yay, it's useful! :D

It definitely is @jfmengels - great work :)

sindresorhus · 2016-10-11T17:33:31Z

package.json

      "quote-props": 0,
-      "import/no-extraneous-dependencies": 0,
      "no-warning-comments": 0,
      "complexity": 0,
      "no-nested-ternary": 0,


Was removed from XO. So can be removed here too.

Thanks 👍

sindresorhus · 2016-10-11T17:33:53Z

package.json

@@ -63,13 +63,14 @@
    "rules": {
      "eqeqeq": ["error", "allow-null"],
      "no-eq-null": 0,
-      "react/prop-types": 0,
-      "babel/new-cap": 0,
+      "new-cap": 0,
      "quote-props": 0,


Was changed to what you want, so can be removed and enabled again.

sindresorhus · 2016-10-11T17:35:32Z

test/index.js

@@ -27,11 +27,11 @@ test.before(async () => {
    path: pathToBinary
  });

-  await app.start();
+  return app.start();


Would be better to use a ESLint ignore comment until it's fixed: avajs/eslint-plugin-ava#147 I would also add a TODO comment about removing the ignore when fixed in eslint-plugin-ava.

I did that at first, but realized that using async/await only for the last promise return was a bit over the top anyway - this looks quite clean: test.after(() => app.stop()).

I don't see how using async/await here is over the top. It works pretty much the same internally (async functions return a promise), but is more readable and easier to work with. What if you suddenly want to do something after app.start()?

eslint-plugin-ava 3.1.1 was just released and should have fixed the issue.

fixed, thanks @jfmengels :)

sindresorhus · 2016-10-11T17:37:37Z

We could change these to default exports, but I don't think it makes it any clearer (rather the opposite).

But does it makes it any worse? I don't think so. I feel it's generally better to use default exports when possible. One thing. One concern.

sindresorhus · 2016-10-11T18:18:18Z

For reference, the XO release notes: https://github.com/sindresorhus/xo/releases/tag/v0.17.0

sindresorhus · 2016-10-11T18:20:59Z

lib/components/split-pane.js

@@ -1,3 +1,4 @@
+/* eslint-disable quote-props */


Just curious, why?

divider_horizontal and family aren't camel case identifiers. Preferably I would've changed it to be divider-horizontal instead, but it would be a bit rash as someone might be using the underscore separated identifiers in a plugin or similar.

The reason it's not dividerHorizontal is this: https://github.com/zeit/hyper/blob/master/lib/components/split-pane.js#L86 (could of course capitalize though)

ekmartin · 2016-10-11T18:28:14Z

Thank you for the feedback.

In regards to the default exports issue I completely agree if the filename matches the intent of the export, however in these cases it really doesn't. Take this as an example. If this should've been a default export I think the file should've been called last as well (like this one), to at least make it a little less confusing. I don't think that makes it any clearer, or easier to use though.

sindresorhus · 2016-10-11T18:40:49Z

app/index.js

@@ -410,7 +410,7 @@ function installDevExtensions(isDev) {
  if (!isDev) {
    return Promise.resolve();
  }
-  const installer = require('electron-devtools-installer'); // eslint-disable-line global-require
+  const installer = require('electron-devtools-installer'); // eslint-disable-line import/no-extraneous-dependencies


Why is this one being ignored?

Hm. Normally when we have dependencies that are used in both the main process and the renderer we duplicate the dependencies to both package.json files. electron-devtools-installer is only used in dev however, and is only imported in the app itself. The problem is that install-app-deps from electron-builder doesn't install devDependencies, so we can't have it there. Not sure what the best solution would be.

It could be a slightly better idea to, instead of turning it off, changing the config of the rule at that position, to turn on the devDependencies option. That way, if someone removes it from the project while it's still being used, you'd still get the error here.

It's a bit akin to nitpicking as it's a very edge case probably, but I think it's a worthwhile change.

sindresorhus · 2016-10-11T18:44:03Z

I would have called the file array-last.js.

* Use parent xo config in app/ * lint: Fix xo 0.17 errors * app: add missing semver dependency

Use parent xo config in app/

dbcea73

ekmartin commented Oct 11, 2016

View reviewed changes

sindresorhus suggested changes Oct 11, 2016

View reviewed changes

ekmartin force-pushed the xo_false branch 3 times, most recently from 0de1d65 to 5d06d6c Compare October 11, 2016 18:15

sindresorhus reviewed Oct 11, 2016

View reviewed changes

ekmartin force-pushed the xo_false branch from 5d06d6c to 185aab2 Compare October 11, 2016 18:42

ekmartin added 2 commits October 12, 2016 21:02

lint: Fix xo 0.17 errors

68afa26

app: add missing semver dependency

73d82e8

ekmartin force-pushed the xo_false branch from 185aab2 to 73d82e8 Compare October 13, 2016 01:03

rauchg merged commit 7a08b1d into master Oct 13, 2016

rauchg deleted the xo_false branch October 13, 2016 01:37

tylerong pushed a commit to tylerong/hyper that referenced this pull request Jul 3, 2017

Fix xo 0.17 errors and use root xo config for app (vercel#859)

9e54cde

* Use parent xo config in app/ * lint: Fix xo 0.17 errors * app: add missing semver dependency

		@@ -1,3 +1,4 @@
		/* eslint-disable import/no-dynamic-require */

Fix xo 0.17 errors and use root xo config for app #859

Fix xo 0.17 errors and use root xo config for app #859

Conversation

ekmartin commented Oct 11, 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

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

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

sindresorhus commented Oct 11, 2016

sindresorhus commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekmartin commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Oct 11, 2016