Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation#10616

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ronami
Copy link
Member

PR Checklist

Overview

This PR addresses#5681 and makes the rule not report on some valid ways to usemodule augmentation.

The rule already has logic that excludes declaration merging in module augmentation from being reported (added in#3959):

functionisExternalDeclarationMerging(
scope:TSESLint.Scope.Scope,
variable:TSESLint.Scope.Variable,
shadowed:TSESLint.Scope.Variable,
):boolean{
const[firstDefinition]=shadowed.defs;
const[secondDefinition]=variable.defs;
return(
isTypeImport(firstDefinition)&&
isImportDeclaration(firstDefinition.parent)&&
isExternalModuleDeclarationWithName(
scope,
firstDefinition.parent.source.value,
)&&
secondDefinition.node.type===AST_NODE_TYPES.TSInterfaceDeclaration&&
secondDefinition.node.parent.type===
AST_NODE_TYPES.ExportNamedDeclaration
);
}

The current logic specifically looks for interface declarations (and not type alias declarations) and requires types to be explicitly exported.

To my understanding, these two restrictions aren't relevant, as these are valid ways to use module augmentation. I'm basing this on thetypescript's handbook and testing this locally (I don't think this can be reproduced on the playground, as TypeScript would show aCannot find module error).

As far as I know, type aliases cannot be used for declaration merging (e.g. defined multiple times) but can be exported from an augmented module (and extend the original module with new type exports).

Testing this locally, it seems that TypeScript is OK with type and interfaces declarations not being explicitly exported, but I couldn't find anything official on this.

I'm far from having a thorough understanding of declaration merging, so I'd be happy to hear opinions on this.

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@ronami!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint.

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedJan 5, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commitc889be9.

CommandStatusDurationResult
nx test eslint-plugin✅ Succeeded5m 53sView ↗
nx test eslint-plugin --coverage=false✅ Succeeded7m 47sView ↗
nx test visitor-keys✅ Succeeded<1sView ↗
nx run types:build✅ Succeeded<1sView ↗
nx test utils✅ Succeeded<1sView ↗
nx test type-utils✅ Succeeded<1sView ↗
nx run-many --target=build --exclude website --...✅ Succeeded25sView ↗
nx test typescript-eslint✅ Succeeded4sView ↗
Additional runs (24)✅ Succeeded...View ↗

☁️Nx Cloud last updated this comment at2025-01-23 20:26:49 UTC

@netlifyNetlify
Copy link

netlifybot commentedJan 5, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitc889be9
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6792a121cb9d390009b32c26
😎 Deploy Previewhttps://deploy-preview-10616--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		1 paths audited
Performance: 98 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

@ronamironami marked this pull request as ready for reviewJanuary 5, 2025 19:32
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesJan 19, 2025
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

I was procrastinating reviewing this pretty intensely because of it touching declaration merging and shadowing... but it's actually really not that much code I think. 👍 from me.

Also ccing@bradzacher in case they have time to take a look too, as Brad'sseen some things I tend to not recall.

ronami reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJan 19, 2025
bradzacher
bradzacher previously approved these changesJan 22, 2025
@JoshuaKGoldberg
Copy link
Member

@ronami the merge conflicts are inno-shadow from your other PR#10593, could you resolve conflicts? I started trying but they look nontrivial.

ronami reacted with thumbs up emoji

@JoshuaKGoldbergJoshuaKGoldberg removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJan 23, 2025
@ronami
Copy link
MemberAuthor

@ronami the merge conflicts are inno-shadow from your other PR#10593, could you resolve conflicts? I started trying but they look nontrivial.

Oh, I didn't noticed this had conflicts. Sure, I've resolved them 👍

@codecovCodecov
Copy link

codecovbot commentedJan 23, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.19%. Comparing base(9e8828b) to head(c889be9).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@##             main   #10616   +/-   ##=======================================  Coverage   87.19%   87.19%           =======================================  Files         450      450             Lines       15632    15632             Branches     4570     4570           =======================================  Hits        13630    13630             Misses       1645     1645             Partials      357      357
FlagCoverage Δ
unittest87.19% <ø> (ø)

Flags with carried forward coverage won't be shown.Click here to find out more.

Files with missing linesCoverage Δ
packages/eslint-plugin/src/rules/no-shadow.ts80.64% <ø> (ø)

@ronami
Copy link
MemberAuthor

ronami commentedJan 23, 2025•
edited
Loading

I'm not sure why CI failed, the error I see isGitHub Actions has encountered an internal error when running your job.. Should I try re-running CI?

Tests seem to pass locally.

Edit: Green, thanks! 😄

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🔥

ronami reacted with rocket emoji
@JoshuaKGoldbergJoshuaKGoldberg merged commit586e7eb intotypescript-eslint:mainJan 24, 2025
64 checks passed
@ronamironami deleted the no-shadow-fix-module-augmentation branchJanuary 25, 2025 09:52
renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull requestJan 29, 2025
| datasource | package                          | from   | to     || ---------- | -------------------------------- | ------ | ------ || npm        | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 || npm        | @typescript-eslint/parser        | 8.21.0 | 8.22.0 |## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27)##### 🩹 Fixes-   **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612))-   **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552))-   **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678))-   **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616))-   **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638))-   **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517))##### ❤️ Thank You-   mdm317-   Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal)-   Ronen Amiel-   YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull requestJan 29, 2025
| datasource | package                          | from   | to     || ---------- | -------------------------------- | ------ | ------ || npm        | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 || npm        | @typescript-eslint/parser        | 8.21.0 | 8.22.0 |## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27)##### 🩹 Fixes-   **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612))-   **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552))-   **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678))-   **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616))-   **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638))-   **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517))##### ❤️ Thank You-   mdm317-   Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal)-   Ronen Amiel-   YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsFeb 2, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

@bradzacherbradzacherbradzacher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: [no-shadow] false positive with augmented modules
3 participants
@ronami@JoshuaKGoldberg@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp