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

[Fix] export: false positive for typescript namespace merging #2375

Merged
merged 2 commits into from Feb 2, 2022

Conversation

magarcia
Copy link
Contributor

Fixes #1964

Fixes false positives like this:

export class Foo { }
export namespace Foo { }
export function Foo() { }
export namespace Foo { }
export enum Foo { }
export namespace Foo { }

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #2375 (3603428) into main (41d4500) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2375      +/-   ##
==========================================
+ Coverage   95.00%   95.03%   +0.02%     
==========================================
  Files          66       66              
  Lines        2703     2718      +15     
  Branches      901      913      +12     
==========================================
+ Hits         2568     2583      +15     
  Misses        135      135              
Impacted Files Coverage Δ
src/rules/export.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41d4500...3603428. Read the comment docs.

@ljharb ljharb force-pushed the fix/typescript-namespace-merging branch from 8aeb950 to e9518c4 Compare January 27, 2022 18:36
@magarcia magarcia marked this pull request as draft January 27, 2022 18:53
@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

Looks like you may need to skip the tests in older eslint versions, or on the older TS parser.

@magarcia
Copy link
Contributor Author

Looks like you may need to skip the tests in older eslint versions, or on the older TS parser.

Sure 😄 I'm not sure how yet. Do you have an example of how to do that?

BTW: I'm setting it as a draft. After a walk/break 😅 , I realized that this fix won't work properly for:

export class Foo { }
export class Foo { }
export namespace Foo { }

I'll have a second look at it tomorrow, it's already quite late in my timezone.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

Here's an example:

...(semver.satisfies(eslintPkg.version, '< 4.0.0') ? [] : [
{ code: 'var x = require("x")', output: 'var x = require("x")', errors: [ { message: IMPORT_MESSAGE }] },
{ code: 'x = require("x")', output: 'x = require("x")', errors: [ { message: IMPORT_MESSAGE }] },
{ code: 'require("x")', output: 'require("x")', errors: [ { message: IMPORT_MESSAGE }] },
{ code: 'require(`x`)',
parserOptions: { ecmaVersion: 2015 },
output: 'require(`x`)',
errors: [ { message: IMPORT_MESSAGE }],
},
{ code: 'if (typeof window !== "undefined") require("x")',
options: [{ allowConditionalRequire: false }],
output: 'if (typeof window !== "undefined") require("x")',
errors: [ { message: IMPORT_MESSAGE }],
},
{ code: 'if (typeof window !== "undefined") { require("x") }',
options: [{ allowConditionalRequire: false }],
output: 'if (typeof window !== "undefined") { require("x") }',
errors: [ { message: IMPORT_MESSAGE }],
},
{ code: 'try { require("x") } catch (error) {}',
options: [{ allowConditionalRequire: false }],
output: 'try { require("x") } catch (error) {}',
errors: [ { message: IMPORT_MESSAGE }],
},
]),

@magarcia magarcia force-pushed the fix/typescript-namespace-merging branch 3 times, most recently from 43394ae to 48d3070 Compare January 28, 2022 12:34
@magarcia magarcia marked this pull request as ready for review January 28, 2022 14:06
src/rules/export.js Outdated Show resolved Hide resolved
@magarcia magarcia force-pushed the fix/typescript-namespace-merging branch from 48d3070 to 443bee0 Compare January 31, 2022 09:45
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.

[export] “Multiple exports” false positive on merged class and namespace
2 participants