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

[no-commonjs] add allowConditionalRequire option #1439

Merged

Conversation

Pessimistress
Copy link

@Pessimistress Pessimistress commented Aug 4, 2019

Fixes #1437

Currently:

  1. if (typeof window !== "undefined") require("x") is reported by default
  2. if (typeof window !== "undefined") { require("x") } is never reported (scope)
  3. typeof window !== "undefined" && require("x") is never reported (conditional require)

After this PR, all cases will be treated the same (as conditional requires). A follow up PR will add an option for users to configure whether conditional requires should be reported.

@coveralls
Copy link

coveralls commented Aug 4, 2019

Coverage Status

Coverage increased (+0.004%) to 96.411% when pulling a60e5c6 on Pessimistress:no-commonjs-conditional-require into 414c923 on benmosher:master.

@Pessimistress Pessimistress force-pushed the no-commonjs-conditional-require branch from a7c63d9 to 654dc10 Compare August 4, 2019 18:48
@Pessimistress Pessimistress changed the title [no-commonjs] report conditional require in a if block [no-commonjs] do not report conditional require in a if statement Aug 4, 2019
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the option may need to be added first, so that those who are relying on these warnings have an existing solution to enable them.

@Pessimistress
Copy link
Author

I have no idea how to add the option without making these changes... Do you mind if I just add it in the same PR?

@Pessimistress
Copy link
Author

those who are relying on these warnings have an existing solution to enable them

I highly doubt if anyone is relying on these warnings. The only breaking change is that if (typeof window !== "undefined") require("x") is no longer reported. Considering if (typeof window !== "undefined") { require("x") } was never reported, I consider the existing behavior already broken.

@ljharb
Copy link
Member

ljharb commented Aug 19, 2019

@Pessimistress adding the option in this PR sounds fine to me.

@Pessimistress Pessimistress changed the title [no-commonjs] do not report conditional require in a if statement [no-commonjs] add allowConditionalRequire option Aug 20, 2019
src/rules/no-commonjs.js Outdated Show resolved Hide resolved
src/rules/no-commonjs.js Outdated Show resolved Hide resolved
src/rules/no-commonjs.js Show resolved Hide resolved
}

function validateScope(scope) {
if (scope.variableScope.type === 'module') return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (scope.variableScope.type === 'module') return true

now that the other change is done :-)

@ljharb ljharb force-pushed the no-commonjs-conditional-require branch 2 times, most recently from 945ee6d to a60e5c6 Compare December 8, 2019 05:46
@ljharb ljharb merged commit a60e5c6 into import-js:master Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[no-commonjs] does not catch conditional require
4 participants