Implement the no-use-before-define ESLint rule #770

aickin · 2016-11-23T20:36:13Z

Turned on the no-use-before-define rule, in reference to bug #763, and changed the files that didn't pass the new listing rule.

I first turned the rule on completely, but renderMiddleware got really hairy, so I went back and used the {functions: false} parameter to the rule. This means that basic functions don't need to be defined in order, which is OK from a functionality perspective because functions are always hoisted. renderMiddleware is a bit of function soup, so setting this parameter let me sidestep it without compromising on correctness. If someone else wants to tackle implementing the rule with {functions:true}, that's cool, but it's also probably fine not to do so, as well.

I just moved functions or variable definitions around in the files without changing their contents, except for two cases, both of which involved circular references, and which probably require closer review:

ReactServerAgent: this one had a function that referred to the API object, and the methods of the API object call the function. I fixed it by inlining and deleting the function.
PageUtil: the exported object, PageUtil, had a method named makeArray, which was referenced as PageUtil.makeArray in a bunch of functions in the file. Those functions, in turn, are members of PageUtil, causing a circular reference. To break that, I made makeArray a top level function; it doesn't use this at all and is a simple no side effects helper, then I changed all the calls to PageUtil.makeArray to makeArray. I left makeArray as a member of PageUtil, because I have no idea if clients of this file use it or not. Also of note but unrelated: I had to move around the variable declarations inside of the PageConfig definition.

Let me know what y'all think!

aickin · 2016-11-23T20:38:47Z

FYI, because I didn't use comma-chained declaration in RootElement.js, this will have merge conflicts with PR #769. If you choose to merge #769 first, I'll happily do the merge; it should not be difficult at all.

gigabo · 2016-11-23T20:41:01Z

packages/react-server-cli/src/compileClient.js

-// once the compiler has been run. The file path returned from the promise
-// can be required and passed in to reactServer.middleware().
-// TODO: add options for sourcemaps.
-export default (opts = {}) => {


Ugh. This is exactly what you said would happen, but it's just as painful as expected.

It's nice to have default exports at the top of modules!

I guess this is the price we pay for the safety this rule provides. 😖

I hear you; I think I was the original author of a few of those files in that format.

Another option is to convert functions in those files from const foo = (bar) => { /* body */} to function foo(bar) {/* body */} and rely on function hoisting, which I think would allow you to put the exports at the top in most cases (but maybe not all? haven't thought it through). Less ES6-y and relies on some hoisting magic, but maybe more readable? (Maybe?)

Yeah, IMO the little bit of hoisting magic there is worth it for the benefit of having the first thing you see when you open a module be the part you're looking for.

OK, gimme a sec. I'll try it out.

gigabo · 2016-11-23T20:43:24Z

this will have merge conflicts with PR #769... I'll happily do the merge

Oh, thanks for pointing that out. Yeah, let's merge that first and have you handle the conflict resolution, since the author over there is a pretty new contributor.

gigabo · 2016-11-23T20:47:13Z

packages/react-server-cli/src/parseCliArgs.js

@@ -2,6 +2,59 @@ import yargs from "yargs"
 import fs from "fs"
 import pem from "pem"

+const sslize = async argv => {


Since this is set up with {functions: false}, would it work to just turn these into normal functions to avoid having to move them above the default exports?

Like, could this stay where it was and turn into:

async function sslize(argv) { ... }

Oh, heh, you had the same thought. 😁

…f moving the functions all around, the PR now converts them to hoisted JavaScript functions. Should disrupt the file organization less.

aickin · 2016-11-23T21:08:24Z

OK, I updated the PR with a version that just converts var foo = function() {} to function foo() {} and depends on hoisting. It's much less disruptive (~300 lines changed vs. ~630 in the previous version), and it allows you to keep default exports at the top of the file if that's what you're into.

There are a couple files (logging/server.js and logging/stats.js) where I converted some functions that didn't strictly need to be converted in order to make the linting rule pass. I thought it was better to do that than to have functions defined a bunch of different ways in the same file.

gigabo · 2016-11-23T21:11:14Z

Oh, man, that's so much better! 😌

gigabo · 2016-11-23T21:13:44Z

I converted some functions that didn't strictly need to be converted in order to make the linting rule pass. I thought it was better to do that than to have functions defined a bunch of different ways in the same file.

Yeah, much nicer that way. Thanks for thinking of that.

gigabo · 2016-11-23T21:18:39Z

packages/react-server/core/util/PageUtil.js

@@ -17,6 +17,85 @@ var {isTheFold, markTheFold} = require('../components/TheFold');
 // These three data structure define the page interface.


+var PageConfig = (function(){


Hmm... this is moving a bunch of private helpers up to the top, where the data structures that define the public interface used to be. This one's trickier, isn't it... 🤔

If nothing else the comments above here should stay with the section with those data structures.

OK. I was unclear if those comments were for clients of this file (in which case they make more sense to me at the top of the file) or folks who are going to muck with the impl of this file (in which I case they definitely belong below). I'll move them!

And, I should say, I skimmed the comments at best ;)

I was unclear if those comments were for clients of this file... or folks who are going to muck with the impl of this file

Both. 😹

Hahahahahaha.

In any case, the PR is updated, and the comment was reunited with the code. Onwards!

@gigabo

…ch, @gigabo.

gigabo · 2016-11-28T20:32:36Z

Okay @aickin I've just merged #769. Please rebase this when you get a chance.

aickin · 2016-11-28T20:43:37Z

Great! It's rebased, and I'll keep an eye on the CI build; let me know if you need anything more.

gigabo · 2016-11-28T22:10:56Z

Thanks @aickin!

Implemented the no-use-before-define rule

b19465e

gigabo reviewed Nov 23, 2016

View reviewed changes

Reworking the PR in response to @gigabo's helpful critique. Instead o…

b623acf

…f moving the functions all around, the PR now converts them to hoisted JavaScript functions. Should disrupt the file organization less.

gigabo reviewed Nov 23, 2016

View reviewed changes

Moving a comment that got separated from its code. Thanks for the cat…

c4832e5

…ch, @gigabo.

Merge branch 'master' into no-use-before-define

eff2cd9

gigabo added the bug An issue with the system label Nov 28, 2016

gigabo merged commit 6686a67 into redfin:master Nov 28, 2016

gigabo mentioned this pull request Dec 1, 2016

line 156 in RootElement.js "var subscribe" function def not hoisted #763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the no-use-before-define ESLint rule #770

Implement the no-use-before-define ESLint rule #770

aickin commented Nov 23, 2016 •

edited

Loading

aickin commented Nov 23, 2016

gigabo Nov 23, 2016

aickin Nov 23, 2016

gigabo Nov 23, 2016

aickin Nov 23, 2016

gigabo commented Nov 23, 2016

gigabo Nov 23, 2016

gigabo Nov 23, 2016

aickin commented Nov 23, 2016

gigabo commented Nov 23, 2016

gigabo commented Nov 23, 2016

gigabo Nov 23, 2016

aickin Nov 23, 2016

aickin Nov 23, 2016

gigabo Nov 23, 2016

aickin Nov 23, 2016

gigabo commented Nov 28, 2016

aickin commented Nov 28, 2016

gigabo commented Nov 28, 2016

		@@ -17,6 +17,85 @@ var {isTheFold, markTheFold} = require('../components/TheFold');
		// These three data structure define the page interface.


		var PageConfig = (function(){

Implement the no-use-before-define ESLint rule #770

Implement the no-use-before-define ESLint rule #770

Conversation

aickin commented Nov 23, 2016 • edited Loading

aickin commented Nov 23, 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

gigabo commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aickin commented Nov 23, 2016

gigabo commented Nov 23, 2016

gigabo commented Nov 23, 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

gigabo commented Nov 28, 2016

aickin commented Nov 28, 2016

gigabo commented Nov 28, 2016

aickin commented Nov 23, 2016 •

edited

Loading