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(utils): add missing fields to flat config types #7933

Merged
merged 1 commit into from Nov 15, 2023

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Nov 15, 2023

Overview

This adds the missing fields from #7273

cc @karlhorky

@bradzacher bradzacher added the bug Something isn't working label Nov 15, 2023
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

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.

Copy link

netlify bot commented Nov 15, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 79500fe
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/655421581acdee00078934f2
😎 Deploy Preview https://deploy-preview-7933--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: 90 (🔴 down 8 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.

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.

TIL "stringly"

@bradzacher bradzacher merged commit 533861a into main Nov 15, 2023
49 checks passed
@bradzacher bradzacher deleted the update-flat-config-types branch November 15, 2023 02:13
@karlhorky
Copy link

Nice, thanks! I'll check this out when this lands in @typescript-eslint/utils@6.11.1 or @typescript-eslint/utils@6.12.0

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Nov 15, 2023

For visibility, we publish canary builds on every commit to main: https://main--typescript-eslint.netlify.app/users/releases#canary.

(not saying you need to try them - just that if anybody urgently wants this ASAP, the canaries are an option 😄)

@karlhorky
Copy link

karlhorky commented Nov 16, 2023

Ok thanks, I decided to try out the canary / alpha releases and upgraded to @typescript-eslint/*@6.11.1-alpha.20 here, and simplified some types:

upleveled/eslint-config-upleveled@bb2a860#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346

Notes:

The reason I'm still casting like this is that I want a runtime error if eslint-plugin-jsx-a11y stops distributing configs.recommended.rules.

// eslint-disable-next-line rest-spread-spacing -- Allow JSDoc casting
.../** @type {Exclude<Exclude<import('@typescript-eslint/utils/ts-eslint').FlatConfig.Plugin['configs'], undefined>[string], undefined>} */ (
  /** @type {Exclude<import('@typescript-eslint/utils/ts-eslint').FlatConfig.Plugin['configs'], undefined>} */ (
    jsxA11y.configs
  ).recommended
).rules,

An alternative that I used lower down (to throw a more helpful error) is to first assign the potentially-undefined config object to a variable and then do some manual testing on it and throw if it's not the right shape, which will narrow the type and allow usage without casting:

const firstConfig = config[0];

if (!firstConfig || !firstConfig.plugins || !firstConfig.rules) {
  throw new Error(
    `SafeQL configuration failed

The UpLeveled ESLint config object does not contain all of the properties: .plugins, .rules`,
  );
}

firstConfig.plugins['@ts-safeql'] = await import('@ts-safeql/eslint-plugin');
firstConfig.rules['@ts-safeql/check-sql'] = [
   // ...

@bradzacher
Copy link
Member Author

@karlhorky FYI you're doing things a little incorrectly here!
Specifically the type of the plugin isn't a FlatConfig.Plugin because the plugin doesn't support flat configs yet.

If you have a look at #7935 you can see how I typed things.

We can probably expose better types here though to make this easier. I.e. We should probably expose a type like FlatConfig.ClassicPlugin<T> where T can be used to define the shared configs.

You also should be using the compat tooling instead of deeply pulling apart the config. I still don't know the "best" way to do this style of "extend multiple configs for a given set of files. I asked on E ESLint discord (https://discord.com/channels/688543509199716507/1174310123791990866) but I got ignored... Might need try pinging again.

@karlhorky
Copy link

karlhorky commented Nov 17, 2023

Yeah, I'm aware I'm faking the types a bit in some places (this is to avoid having to know / keep updated on which plugins expose which type of config). Maybe I should open PRs to add types to each of the plugins I use... 🤔 so that I don't need to maintain them in my project - and then all consumers will have types.

This ESLint config is pretty runtime-tested as is (in many student projects), so it's mostly just making the type system happy and reducing the faking to the least amount possible.

But I will for sure watch all new things including #7935 and use anything to make it simpler!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants