Proposal: Add option to allow sync methods at root level in no-sync

[robertrossmann]

Current state

In Node.js, requiring or importing a file/module is synchronous. Therefore, making more synchronous calls does not hurt anyone, it just slows the loading process down a bit. I believe the rule's intention is to warn people if they accidentally or intentionally make a blocking call in a place where it could seriously hurt performance (ie. in a hot code path etc.).

Currently, the no-sync rule is either enabled or disabled, ie. either you can use sync methods or you cannot.

Intended state

A new option is introduced to the no-sync rule: allowAtRootLevel (bool, default false) (exact name is subject to discussion, I don't really think this is appropriate). With this option turned on, ESLint would not warn on sync calls appearing at the root level of the file.

Examples

// no-sync: [1, { allowAtRootLevel: true }]
// index.js
// ok - appears at root of the module
const contents = fs.readFileSync('/super/important.txt')

module.exports = function getImportant() {
  return contents
}

// no-sync: [1, { allowAtRootLevel: true }]
// index.js
module.exports = function getImportant() {
  // not ok - not at root level! (no change to current behaviour)
  return fs.readFileSync('/super/important.txt')
}

The goal is to avoid turning this rule off on a per-line basis at the root of the module when ie. contents of a file are required for the module to operate.

[platinumazure]

To be clear, does "the root of the module" mean the module scope, outside of function scopes? Or should block scopes be factored in as well?

Examples:

fs.readFileSync('file.txt');     // very clearly root of the module

function someFunction() {
    fs.readFileSync('file.txt'); // very clearly not root of module
}

if (foo) {
    fs.readFileSync('file.txt'); // in block scope but not in function scope, is this root of module?
}

[robertrossmann]

@platinumazure In any place where we can assert with 100% accuracy that the sync call will be executed as part of the module loading process.

Unless I am missing something, this implies anywhere except function scope.

[robertrossmann]

Is there anything I could do to move this proposal forward? Thanks!

[robertrossmann]

I would really love to get this accepted and perhaps even open up a PR - could someone accept this proposal? Thanks!

cc @platinumazure

[platinumazure]

@robertrossmann Thanks for following up! We need the rest of the team to take a look. Unfortunately it looks like this just completely slipped through the cracks, and for that I apologize.

This enhancement seems reasonable to me (helps that it proposes an option rather than change in default behavior), so I'll champion this.

@eslint/eslint-team Could we give this a review? Just need three 👍s or, if this doesn't seem like a good enhancement, let's talk it out.

[robertrossmann]

I am still interested in this! 😀

Any chance of getting this approved? I know you are probably busy working on 4.0. Thanks!

[qfox]

I like this too. Seems pretty reasonable and logical.

I'm just scared a little about this:

doSomething();
getImportant();
doSomethingElse();

// ... somewhere at the end of file
function getImportant() {
  // Should this be treated as ok if there are no uses in other places (let's say no exports of this function)?
  return fs.readFileSync('/super/important.txt')
}

and this:

(function() {
  // it's still a root level
  (function() { /* and this too */ }());
}());

I'm pretty sure someone will want to do that.

p.s. Perhaps we can stick to sync calls at module root and IIFE level.

[not-an-aardvark]

This now has 3 👍s and a champion. Marking as accepted.

[VictorHom]

If no has started any work, can I jump in to help out with a pr?

[not-an-aardvark]

Feel free!

[robertrossmann]

Amazing, thanks!