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

feat(utils): update types to reflect RuleContext and SourceCode changes and deprecations #7812

Merged
merged 8 commits into from Nov 13, 2023

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Oct 20, 2023

PR Checklist

Overview

Reference: https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @G-Rath!

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

netlify bot commented Oct 20, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit d3bd6c3
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/655151c2d39d0f0009fbb413
😎 Deploy Preview https://deploy-preview-7812--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: 89 (🔴 down 10 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.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 20, 2023

Are folks ok if I ignore the deprecation linting errors? I can help getting them addressed but would prefer to do so in dedicated PR since they'll require actual runtime changes and there's a few different ways they can be approached

@JoshuaKGoldberg
Copy link
Member

Well, we need typescript-eslint to work on both v8 and v9 for the time being. Dropping v8 would be a breaking change. I'd presume eslint-disable comments would be the way to go.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 21, 2023

My point is you'll need to make changes to support v9 since some of these are being removed in that major - it's pretty straight forward (I've already done this in eslint-plugin-jest) but I'd still prefer to do it in a dedicated PR because there are two different changes here (adding types, moving off deprecated functions and properties) & how I've done it for eslint-plugin-jest might not be how you want to do it here

I'll add disable directives for now

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Oct 22, 2023

Agreed. https://github.com/jest-community/eslint-plugin-jest/blob/3c12c9842df7950dd214855fa69b1785c4d1574d/src/rules/utils/misc.ts is roughly the strategy I'm betting we'll want to take. 🙂

export const getDeclaredVariables = (
  context: TSESLint.RuleContext<string, unknown[]>,
  node: TSESTree.Node,
) => {
  const sourceCode = getSourceCode(context);

  if ('getDeclaredVariables' in sourceCode) {
    return sourceCode.getDeclaredVariables(node);
  }

  return context.getDeclaredVariables(node);
};

Copy link
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.

💯 thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Oct 22, 2023
@JoshuaKGoldberg
Copy link
Member

Adding 1 approval label for now because this I'd love someone else from @typescript-eslint/triage-team to take a look at this area code. Both to raise visibility, and because I'm not 100% confident I'm not missing something. A gut hunch.

@JoshuaKGoldberg JoshuaKGoldberg merged commit b73d8b2 into typescript-eslint:main Nov 13, 2023
45 checks passed
@JoshuaKGoldberg
Copy link
Member

Thanks again @G-Rath!

@JoshuaKGoldberg
Copy link
Member

Amusingly, there are now a bunch of lint failures on main: #7913. But this PR had a passing ✅. So nobody should feel bad 😄

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
@G-Rath G-Rath deleted the deprecate-getAncestors branch January 4, 2024 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Update types for ESLint v9's changes and deprecations
4 participants