Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: options to ignore try,catch for rule global-require #3834

Closed
evilebottnawi opened this issue Sep 17, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@evilebottnawi
Copy link
Contributor

commented Sep 17, 2015

Example:

try {
    posix = require('posix');
} catch (ex) {
    // Nothing
}

Now i get error global-require

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot

This comment has been minimized.

Copy link

commented Sep 17, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Sep 17, 2015

@btmills btmills added enhancement rule and removed triage labels Sep 17, 2015

@btmills

This comment has been minimized.

Copy link
Member

commented Sep 17, 2015

This pattern is explicitly disallowed in the docs, but I've seen it before when trying to import an optional dependency. @evilebottnawi what would such an option look like?

@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2015

@btmills i see two ways import optional dependencies.
First:

try {
    posix = require('posix');
} catch (ex) {
    // Nothing
}

Second:

try {
    var posixPath = require.resolve('posix');

    if (posixPath) {
        posix = require('posix');
    }
} catch (exception) {
    // Nothing
}

I think first way is better, less code and faster.
I think add options ignoreTryCatch.

@btmills

This comment has been minimized.

Copy link
Member

commented Sep 17, 2015

var moduleName = 'preferred';
try {
    require.resolve(moduleName);
} catch (err) {
    moduleName = 'fallback';
}

var optionalModule = require(moduleName);

That works today, but it's ugly.

@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2015

@btmills What is required is not always the fallback

@nzakas nzakas added the evaluating label Sep 18, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

I'm not sure an exception makes sense here. It seems like a very special case that might best be handled via configuration comments.

@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2015

@nzakas maybe then it can be put in the documentation

@nzakas

This comment has been minimized.

Copy link
Member

commented Sep 21, 2015

What would you suggest be put in the documentation?

@jamestalmage

This comment has been minimized.

Copy link

commented Sep 26, 2015

If nothing else the documentation should use a different word than "scope"

Disallow require() outside of the top-level module scope

To me that infers variable scope, but most of the warn examples have nothing to do with variable scope (try blocks included).

The argument regarding the performance implications of require only apply if it is actually in a nested variable scope (a function which can be deferred).

I personally see nothing wrong with a require inside a try block or if/then (switch seems weird, but whatever). It would be nice to have an option that only complained about require in new variable scopes.

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Dec 21, 2015

I think the convo above suggests to put something in the docs. is that correct?
CC @nzakas

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Yes. We should basically say that this rule mimics the behavior of ES6 modules imports.

@btmills

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

Working on this.

ilyavolodin added a commit that referenced this issue Feb 8, 2016

Merge pull request #5180 from eslint/issue3834
Docs: Clarify global-require inside try/catch (fixes #3834)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.