Repository navigation
require unification proposal #14041
Description
Activity
Tbh, I find that rule superfluous, and maybe deleting it from the doc is the easiest approach. If our tests required enough modules to not immediately see which line requires which module, maybe, but that’s hardly true.
Also, it could actually be good not to always follow it; require()ing core modules is not always free of side-effects, so testing different orders might not be a bad thing.
Reacted by Vse Mozhe Buty, Rich Trott and Dmitry RumyantsevFWIW, refs: #10616
@sam-github makes a good point:
So maybe amend that rule to apply only when more than N requires are used (let's say N=5)?
Also as a follow up, what do we recommend exactly regarding destructuring?
Suggestion:If more than two functions from the module are used, destructuring is recommended (unless the functions have very generic names).So maybe amend that rule to apply only when more than N requires are used (let's say N=5)?
...
If more than two functions from the module are used, destructuring is recommended (unless the functions have very generic names).I prefer that we have fewer and simpler rules. I'd rather leave these things to each individual's judgment than have multiple "if N is greater than 4, do X otherwise do Y" rules.
When the "alphabetize the modules" rule was first suggested, I was neutral. Since then, I've become -0 on it. It seems low value to me, at least in tests. Most tests don't follow the rule. It's one more thing newcomers have to learn. And I generally dislike rules for code that go unflagged by our tooling.
So I'd be OK with just removing the rule, especially if the alternative is low-value churn in hundreds of files.
Reacted by Anna Henningsen and Gibson FahnestockI think the usual rationale behind sorting dependencies is to eliminate duplications. Something we currently don't lint for.
This passes our linter:const { exec } = require('child_process'); const { spawn } = require('child_process'); const child_process = require('child_process'); const cp = require('child_process'); console.log(child_process); console.log(cp);
I'll try to find a way to lint for that.
Reacted by Vse Mozhe Buty and Anna HenningsenJust to point out, while its true that most of our current tests don't alphabetize requires, its also true that most don't have a descriptive comment. The point of the test guide wasn't to encode current practice, but to describe what we thought it should be. And one point of the sorting which isn't effected at all by whether there is one or 20 requires is to make modification easy, so when a require is added maintainers don't have to try to deduce the personal preference of the original author: new on the top, new on the bottom, grouped by some "I feel these are similar" criteria, random, ...?
Which is why even though I personally dislike
const {exec, spawn} = require('child_process');because it breaks the "write code that git diffs well" guideline, I will happily code that way in node because it means I don't have to think about it, and be even happier if lint tells me what the guidelines are when I break them.Reacted by Vse Mozhe Buty, Rich Trott and Gibson FahnestockIt seems this is a questionable approach, so I shall close for now. Thank you for the feedback.
Reacted by Refael Ackermann
"How to write a test..." guide advises
requirealphabetical sorting for new tests, but our codebase mostly does not follow this rule (some logical or random order is used).I can try to revisit all
requiresections in benchmarks, libs, and tests. And while I am at it, all these things also can be addressed:requirein one place when it is appropriate (no conditionals involved).requiresection (not sure if any rules like this or this ones are required, but at least this can be unified).However, this will be a time-consuming task, so I need to be sure if: