Skip to content

fix(visitor-keys): update keys for ImportAttribute#10649

Merged
bradzacher merged 2 commits intotypescript-eslint:mainfrom
antfu:fix/vistor-keys
Feb 17, 2025
Merged

fix(visitor-keys): update keys for ImportAttribute#10649
bradzacher merged 2 commits intotypescript-eslint:mainfrom
antfu:fix/vistor-keys

Conversation

@antfu
Copy link
Copy Markdown
Contributor

@antfu antfu commented Jan 14, 2025 •
edited
Loading

PR Checklist

Overview

Sorry for a quick PR without an issue - I will fill that later. The key assertions on ImportDeclaration as renamed to attributes but the visitor keys are not updated. This causing the ImportAttribute listener in ESLint rule to be called twice with the same node because eslint's visitor keys now include attributes:
https://github.com/eslint/js/blob/79f6f9a603e44707a7774ebd3f871da336242fb5/packages/eslint-visitor-keys/lib/visitor-keys.js#L148

Note: I noticed that the keys are actually merged with eslint-visitor-keys, as attributes is already in eslint-visitor-keys, should I remove them instead? If so, there seem to be quite a few keys that are also redundant. -> I made #10650 for the record

@typescript-eslint
Copy link
Copy Markdown
Contributor

typescript-eslint Bot commented Jan 14, 2025

Thanks for the PR, @antfu!

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 on https://opencollective.com/typescript-eslint.

@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 14, 2025 •
edited
Loading

✅ Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 0a04812
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/679a3f83fde6500008bc447c
😎 Deploy Preview https://deploy-preview-10649--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: 92 (🔴 down 6 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 your Netlify site configuration.

@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Jan 14, 2025 •
edited
Loading

View your CI Pipeline Execution ↗ for commit 0a04812.

Command Status Duration Result
nx test eslint-plugin --coverage=false ✅ Succeeded 7m 32s View ↗
nx test eslint-plugin ✅ Succeeded 5m 55s View ↗
nx test visitor-keys --coverage=false ✅ Succeeded 3s View ↗
nx test utils ✅ Succeeded 4s View ↗
nx run types:build ✅ Succeeded <1s View ↗
nx run-many --target=build --exclude website --... ✅ Succeeded 47s View ↗
nx test typescript-estree ✅ Succeeded 59s View ↗
nx test typescript-estree --coverage=false ✅ Succeeded 1m 16s View ↗
Additional runs (24) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-01-29 15:07:21 UTC

@antfu antfu mentioned this pull request Jan 14, 2025
Closed
3 tasks
@JoshuaKGoldberg
Copy link
Copy Markdown
Member

JoshuaKGoldberg commented Jan 23, 2025

cc @fisker @ronami from #10640 -> #10691: is there more work to be done in #10691?

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Jan 23, 2025
Merged
3 tasks
@antfu antfu mentioned this pull request Jan 29, 2025
Merged
JoshuaKGoldberg
JoshuaKGoldberg approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! I just want to be thorough and do research on this, since it's a tricky change. It looks good to me. Just to be safe I think another review from @typescript-eslint/triage-team would be good.

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Feb 17, 2025
@bradzacher bradzacher merged commit faec1ca into typescript-eslint:main Feb 17, 2025
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

@bradzacher bradzacher bradzacher approved these changes

Assignees

No one assigned

Labels

1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@antfu @JoshuaKGoldberg @bradzacher