tests(lint): require file extension in require() #9017

brendankenny · 2019-05-22T01:28:58Z

enforce change made in #9006

creates a local eslint rule that requires a local path in a require() statement includes the file extension. eslint has support for local rules on the command line via --rulesdir, but that means it won't work in text editors with an eslint plugin installed.

eslint-plugin-local-rules is an eslint plugin that gets around that by just loading a local file as its own payload. The downside is that it has to be a root-level file, but on the plus side it works for our inherited .eslintrc.js files for the clients and viewer.

(there's another similar plugin eslint-plugin-rulesdir that allows specifying the location of the file, but I'm not sure if it inherits and it appears like it might be broken in vscode (eslint-community/eslint-plugin-rulesdir#2), something that's been tested and is working in eslint-plugin-local-rules (cletusw/eslint-plugin-local-rules#1))

happy to bikeshed on how we do this

(also picks up a few places that #9006 missed and deletes dot-js.js as hopefully we don't need any more mass fixes, but I can restore if we want to keep it)

brendankenny · 2019-05-22T01:34:43Z

deletes dot-js.js as hopefully we don't need any more mass fixes

Hmm, we could use this for a returned fix() method. Does anyone ever actually use --fix with eslint?

connorjclark · 2019-05-22T01:39:35Z

deletes dot-js.js as hopefully we don't need any more mass fixes

Hmm, we could use this for a returned fix() method. Does anyone ever actually use --fix with eslint?

all the time. how else do you format?

patrickhulce

eslint is my preferred path too, how hard is it to write a fixer for a rule?

patrickhulce · 2019-05-22T02:50:13Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -9,7 +9,7 @@ const BaseNode = require('../base-node.js');
 const TcpConnection = require('./tcp-connection.js');
 const ConnectionPool = require('./connection-pool.js');
 const DNSCache = require('./dns-cache.js');
-const mobileSlow4G = require('../../../config/constants').throttling.mobileSlow4G;


I am so curious how these slipped through haha

the answer here is I was conservative and only matched require(...); and since these do require(...).someVariable they didn't match :)

patrickhulce · 2019-05-22T02:50:50Z

lighthouse-core/index.js

@@ -70,7 +70,7 @@ function generateConfig(configJson, flags) {

 lighthouse.generateConfig = generateConfig;
 lighthouse.getAuditList = Runner.getAuditList;
-lighthouse.traceCategories = require('./gather/driver').traceCategories;
+lighthouse.traceCategories = require('./gather/driver.js').traceCategories;
 lighthouse.Audit = require('./audits/audit.js');


yikes 😬 those probably need to be fixed and tests added that these don't throw?

yikes 😬 those probably need to be fixed and tests added that these don't throw?

I think this is a valid path? I'm not getting the context of the 😬here :)

hm, this is super weird in the email and on disk I see

lighthouse.Audit = require('./audits/audit.js');

but in GitHub UI I see

lighthouse.Audit = require('./audits/audit.js./audits/audit.js');

seems like a GitHub bug that needs fixing not our bug! 😆

patrickhulce · 2019-05-22T02:53:11Z

lighthouse-core/scripts/dot-js.js

-        if (file.endsWith('.json')) continue;
-        const fullPathOfDep = path.resolve(dir, file);
-        if (!fs.existsSync(fullPathOfDep)) {
-          console.warn(`Uh-oh! Cannot find dependency "${fullPathOfDep}" in ${name}`); // eslint-disable-line no-console


maybe we should run this line over the codebase without the '.js' and .json checks to see if it broke anything else?

maybe we should run this line over the codebase without the '.js' and .json checks to see if it broke anything else?

I was hoping tsc would catch any of those since it usually complains about "Cannot find module", but I can definitely run that over the codebase

seems like this was all false alarms from the GH UI bug lol, so you can feel free to ignore that comment

brendankenny · 2019-05-22T17:37:31Z

all the time. how else do you format?

haha, I guess the red underlines in my editor leave me unable to continue until I fix things, so it never gets too bad

how hard is it to write a fixer for a rule?

I don't think it'll be hard, it can mostly rip off what's done in dot-js.js

brendankenny · 2019-05-22T20:26:32Z

updated with --fix support.

To test, I reverted the changes in #9006 and this PR and verified that the required paths were restored to the same fixed state (only had to add back the eslint-disable max-len comments to get back to exactly the same state)

brendankenny · 2019-05-22T20:32:12Z

lighthouse-core/scripts/lantern/run-once.js

-const traceSaver = require('../../../lighthouse-core/lib/lantern-trace-saver.js');
+const PredictivePerf = require('../../audits/predictive-perf.js');
+const Simulator = require('../../lib/dependency-graph/simulator/simulator.js');
+const traceSaver = require('../../lib/lantern-trace-saver.js');


the only path changes that aren't adding a suffix because these files are already under lighthouse-core/ :)

The lint rule actually won't flag or fix this case normally (since the paths do have a file extension and they resolve to a file), but these were changed when I was testing on the reverted #9006 (which added .js to them) and it seems worth keeping the simplified form

brendankenny · 2019-05-22T20:32:43Z

lighthouse-core/test/fixtures/invalid-gatherers/require-error.js

@@ -6,6 +6,7 @@
 'use strict';

 // NOTE: this require path does not resolve correctly.
+// eslint-disable-next-line local-rules/require-file-extension


these are broken on purpose

brendankenny · 2019-05-22T20:34:49Z

lighthouse-core/test/lib/dependency-graph/simulator/tcp-connection-test.js

@@ -5,7 +5,6 @@
 */
 'use strict';

-// eslint-disable-next-line


here and in network-analyzer-test.js were fixed in #9006 but the new --fix missed these because of these disable comments. I assume these were originally disabled for max-len but maybe moving files reduced the path length over time. It's fine without the disable now

patrickhulce · 2019-05-22T21:49:38Z

tsconfig.json

@@ -18,6 +18,7 @@
    "clients/**/*.js",
    "build/**/*.js",
    "./types/**/*.d.ts",
+    "*.js",


what prompted this?

type checking ./eslint-local-rules.js. I can narrow it if it messes with people's local, untracked scripts :)

yes please if we can :)

patrickhulce

LGTM! yay for cleaner PR reviews :D

patrickhulce · 2019-05-22T22:05:31Z

eslint-local-rules.js

+
+        context.report({
+          node: node,
+          message: 'Required path must have a file extension.',


super nit but this message felt a bit weird to me since Required is so typically used for Necessary/Mandatory

brainstorm of alternatives

Path of module must have a file extension
Path of required module must have a file extension
Require path must have a file extension
Local require path must have a file extension
Path to require must have a file extension
Local path to require must have a file extension

patrickhulce · 2019-05-22T22:06:45Z

eslint-local-rules.js

+        if (!resolvedRequiredPath) {
+          return context.report({
+            node: node,
+            message: `Cannot resolve module '${requiredPath}'.`,


FWIW, the eslint messages I see don't end in periods

FWIW, the eslint messages I see don't end in periods

interestingly eslint seems to take care of this itself. e.g.

let asdf = 'asdf';

when hovering in vscode give

'asdf' is assigned a value but never used. Allowed unused vars must match /(^_$)/.
'asdf' is never reassigned. Use 'const' instead.

(with periods) and they do have periods in the source.

But then in the CLI output, you get

error 'asdf' is assigned a value but never used. Allowed unused vars must match /(^_$)/
error 'asdf' is never reassigned. Use 'const' instead

(no periods).

The same thing happens to this message, so I guess they auto-trim periods when printing? Pretty funny.

woah that's awesome haha

patrickhulce · 2019-05-22T22:08:32Z

eslint-local-rules.js

+const requireFileExtension = {
+  meta: {
+    docs: {
+      description: 'disallow require() without a file extension',


or wait, is this the message that shows up in eslint CLI?

I think this is actually if you generate docs for the rule?

On the command line you just get error Local require path must have a file extension

patrickhulce · 2019-05-22T22:10:18Z

eslint-local-rules.js

+        // a suffix to the existing path, but sometimes humans write confusing
+        // paths, e.g. './lighthouse-core/lib/../lib/lh-error.js'. To cover both
+        // cases, double check that the paths resolve to the same file.
+        const resolvedFixedPath = requireResolveOrNull(fixedPath, contextDirname);


is there a reason all this is outside fix() if we only use it for the fix case?

is there a reason all this is outside fix() if we only use it for the fix case?

Good point, it'll save time in the non --fix case if it's inside fix(), so I'll move it.

Good point, it'll save time in the non --fix case if it's inside fix(), so I'll move it.

well, only if there are a bunch of resolvable requires that don't have a file extension, and I'd imagine most of them would be hitting the require cache, so in practice it won't really save any time, but it looks less overwhelming inside fix() than I thought it would and it makes sense to keep it together

yeah I wasn't really thinking about speed, just relatedness 👍

tests(lint): require file extension in require()

0152de8

brendankenny requested review from patrickhulce and paulirish as code owners May 22, 2019 01:28

patrickhulce reviewed May 22, 2019

View reviewed changes

add path check, fix()

d48aba6

vercel bot deployed to staging May 22, 2019 20:23 View deployment

brendankenny commented May 22, 2019

View reviewed changes

patrickhulce reviewed May 22, 2019

View reviewed changes

patrickhulce approved these changes May 22, 2019

View reviewed changes

feedback

cec48cd

vercel bot deployed to staging May 23, 2019 20:46 View deployment

brendankenny merged commit df85d68 into master May 23, 2019

brendankenny deleted the lintextension branch May 23, 2019 21:42

paulirish pushed a commit that referenced this pull request May 28, 2019

tests(lint): require file extension in require() (#9017)

404dc88

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(lint): require file extension in require() #9017

tests(lint): require file extension in require() #9017

brendankenny commented May 22, 2019

brendankenny commented May 22, 2019

connorjclark commented May 22, 2019

patrickhulce left a comment

patrickhulce May 22, 2019

patrickhulce May 22, 2019

patrickhulce May 22, 2019

brendankenny May 22, 2019

patrickhulce May 22, 2019

patrickhulce May 22, 2019

brendankenny May 22, 2019

patrickhulce May 22, 2019

brendankenny commented May 22, 2019

brendankenny commented May 22, 2019

brendankenny May 22, 2019

brendankenny May 22, 2019

brendankenny May 22, 2019

patrickhulce May 22, 2019

brendankenny May 22, 2019

patrickhulce May 22, 2019

patrickhulce left a comment

patrickhulce May 22, 2019

patrickhulce May 22, 2019

brendankenny May 23, 2019

patrickhulce May 23, 2019

patrickhulce May 22, 2019

brendankenny May 23, 2019

patrickhulce May 23, 2019

patrickhulce May 22, 2019

brendankenny May 23, 2019

brendankenny May 23, 2019

patrickhulce May 23, 2019

tests(lint): require file extension in require() #9017

tests(lint): require file extension in require() #9017

Conversation

brendankenny commented May 22, 2019

brendankenny commented May 22, 2019

connorjclark commented May 22, 2019

patrickhulce 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

Choose a reason for hiding this comment

brendankenny commented May 22, 2019

brendankenny commented May 22, 2019

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

patrickhulce 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

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