8000 Enhancement: Move RuleCreator into its own package with fewer dependencies than utils · Issue #10383 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Enhancement: Move RuleCreator into its own package with fewer dependencies than utils #10383

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.

Sign up for GitHub

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

Jump to bottom
Open
4 tasks done
JoshuaKGoldberg opened this issue Nov 24, 2024 · 19 comments
Open
4 tasks done

Enhancement: Move RuleCreator into its own package with fewer dependencies than utils #10383

JoshuaKGoldberg opened this issue Nov 24, 2024 · 19 comments
Labels
enhancement New feature or request team assigned A member of the typescript-eslint team should work on this.

Comments

@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Nov 24, 2024 •
edited
Loading

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

utils

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Coming over from ArnaudBarre/eslint-plugin-react-refresh#62 (comment): @typescript-eslint/utils has quite a few dependencies that are not necessary for RuleCreator: ts-api-utils, @typescript-eslint/scope-manager, @typescript-eslint/typescript-estree. Even once JoshuaKGoldberg/ts-api-utils#573 is fixed that's >1 MB!

It does seem unfortunate to me that if you want to use typescript-eslint's RuleCreator, you have to pull in multiple other packages beyond @typescript-eslint/utils. Some plugins are intended to work in JS(X) environments. Unlike with TS(X), those users might not already have @typescript-eslint/*, ts-api-utils, typescript, etc. already installed.

Proposal: how about we split out a new package that exports RuleCreator and its relevant types? That way users can import { RuleCreator } without taking a dependency on scope-manager, typescript-estree, etc.

I'm not sure what we'd want to call it:

  • @typescript-eslint/rule-creator: but per notes later, I think we'll also want to export module stuff
  • @typescript-eslint/plugin-kit: in isolation I like this name, but @eslint/plugin-kit has the similar name and is about a completely different idea of "plugins"

For now, I'm going to suggest @typescript-eslint/plugin-kit.

Additional Info

Note that "relevant types" will include RuleModule, which at the moment is stored in @typescript-eslint/utils and accessed as TSESLint.RuleModule. I think we'd want to move RuleModule and its associated types to @typescript-eslint/plugin-kit.

For reference, the package storing the typescript-estree AST types is @typescript-eslint/types, which is 173KB per https://pkg-size.dev/@typescript-eslint%2Ftypes?no-peers. Roughly every plugin I remember seeing does have some internal handling for TS AST types - including eslint-plugin-react-refresh (e.g. handling TSEnumDeclaration, handling type assertions).

Also note that I think there's a desire to make plugins not have a full dependency on AST types at all. If a plugin is meant to be used in ESLint config files, it probably only wants to export types for the plugin and its rules + options. I think that means the @typescript-eslint/plugin-kit package will need to be careful with dependencies:

  • It would be a full dependency on any package that uses it: since RuleCreator is a runtime construct
  • It would have an optional peer dependency on @typescript-eslint/types: since AST types would only be relevant during development, and plugin exports would not include any AST info

This was prompted by @ArnaudBarre noting in ArnaudBarre/eslint-plugin-react-refresh#62 (comment) a desire to have nicely typed rule options for a plugin without many megabytes of added dependencies. Thanks for the prod @ArnaudBarre!

💖
@JoshuaKGoldberg JoshuaKGoldberg added enhancement New feature or request triage Waiting for team members to take a look labels Nov 24, 2024
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Nov 24, 2024
Closed
@ArnaudBarre
Copy link
Contributor

Thanks for taking my question very seriously! I didn't expect you to suggest to re-organize your packages for this!
I think 3 MB is an ok trade off for me, given that the number of people not already using TS-ESLint as a parser is small.
But yeah if you can help others packages make it easy to ship typed configs and rules (with options!) for people using tseslint.config, that's something that would be good foe the community

@bradzacher
Copy link
Member
bradzacher commented Nov 24, 2024 •
edited
Loading

Honestly - I hate having a single utils package.
We should split it up into n packages - one for each conceptual grouping.

Plugin utils.
Ast predicates.
Type predicates.
Etc

Each namespace really should be it's own package to enable wider and lighter reuse.

The difficult thing is documenting this so that people can easily discover the right package to use.

@JoshuaKGoldberg
Copy link
Member Author

I do really like the idea of having granular packages. It'd also make automating docs the way we did in #9293 & #9875. One clean docs section per package instead of a big semi-organized scatter for all of utils.

However, I am concerned that users would find that separation inconvenient for the utils other than plugin-kit? How often would a consumer want to have only, say, AST predicates - and not also type predicates? What's that user story?

Surfacing for reference: @typescript-eslint/type-utils exists right now split out of @typescript-eslint/utils. From its docs, that's to spare that utils from needing a dependency on typescript.

@JoshuaKGoldberg
Copy link
Member Author

I asked in the ESLint Discord about naming. Confirmed that plugin-kit is already a package name intending one thing and we should find another name: https://discord.com/channels/688543509199716507/1133415394925887618/1310631395965669416

Maybe... @typescript-eslint/builders? 🤷 I'm drawing a blank.

@bradzacher
Copy link
Member

@typescript-eslint/plugin-utils?
But also depends on how granular you want to go with the packages.
I.e. you could do @typescript-eslint/rule-creator

@JoshuaKGoldberg
Copy link
Member Author

I like plugin-utils. It mirrors type-utils, etc...

Prediction: I think we'll probably want to keep the rule utils and plugin utils in the same package. I think most users of one will also be users of the other. And, if they're not, we probably want to encourage them to be.

@kirkwaiblinger
Copy link
Member

If we have a utils for every pacakge does it even need to be a separate package? Or can rule-tester just have a ./utils export? Do the utils have too much bloat compared to the main package's thing?

@bradzacher
Copy link
Member
bradzacher commented Nov 25, 2024 •
edited
Loading

What I was suggesting is that we currently have these import paths:

  • /rule-tester
  • /scope-manager
  • /type-utils
  • /utils
  • /utils/ast-utils
  • /utils/eslint-utils
  • /utils/ts-eslint
  • /utils/ts-utils

At least this does map well to something like

  • /ast-utils
  • /eslint-utils
  • /rule-tester
  • /scope-manager
  • /ts-eslint
  • /ts-utils
  • /type-utils
  • /utils

Where:

  • /utils/ast-utils ==> /ast-utils
  • /utils/eslint-utils ==> /eslint-utils
  • /utils/ts-eslint ==> /ts-eslint
  • /utils/ts-utils ==> /ts-utils

But I would think that as part of this we can do a complete refactor of things and both rename everything and split 8000 everything up. I.e. minor release -> keep /utils the same shape then major release -> break it all.

We could target the breaking change soon as well given the next major is planned to be a smaller one based on what we talked about with how we wanted more majors.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Nov 26, 2024
Merged
@rubiesonthesky
Copy link
Contributor

Could rule-kit work for package name for the things mentioned in first messages? Kit sounds that it’s more ready to use and offers helper for whole thing. Utils sound more like just collection of things that you need to then figure out yourself.
I’m not familiar in which category the code would fall. And as pointed out, utils would be in line with other exciting things. I kinda just liked the sound of kit and wanted to comment on that.

@bradzacher
Copy link
Member

The difficult thing is that a subset of utils is still a set of utils. Until you divide that set to a single member it is a set of utils.

Which means that ultimately it will be some subset of utils that ends up in this new package. Which would be confusing for people - "what's the difference between /utils and /new-package? Why should I use the smaller package? Should I be avoiding the larger package and rewrite its pieces to avoid using it?"

The point I'm making is that we have to be very conscious about the divisions we're choosing cos the last thing we want is for most users to go from using one package in /utils to using all of the new packages. If we split something into a new package it should be because it is a distinct usecase or have some clearly defined boundary.

@JoshuaKGoldberg
Copy link
Member Author

rule-kit

The problem with rule-* is that it's not just for rules 🫤. It's also for modules/packages.

I do like the -kit part though 😄

@G-Rath
Copy link
Contributor
G-Rath commented Dec 9, 2024

oohh really excited about this - I've have it on my mind for years but never worked up the courage to make an issue as I figured it would be a big ask to have you awesome volunteers split packages up even further, having to decide "what does this new thing belong in", etc.

The reason it's been on my mind is that we use @typescript-eslint/utils in eslint-plugin-jest and co for the rule creator and the AST types/enums at runtime, but because of the peer dependencies that results in TypeScript technically being required - I don't consider that the end of the world (especially as everything I do these days in JS land is already TS based, and I think that's true for a lot of others too) but it still always niggled me that TypeScript would end up in peoples dependency trees even though it was never needed by us.

So personally I'd like ideally to have access to the rule creator and the AST types/enums by themselves

(to be clear, I'm talking about the imports RuleCreator, AST_NODE_TYPES and AST_TOKEN_TYPES)

@JoshuaKGoldberg JoshuaKGoldberg added team assigned A member of the typescript-eslint team should work on this. and removed triage Waiting for team members to take a look labels Dec 14, 2024
@JoshuaKGoldberg JoshuaKGoldberg self-assigned this Dec 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Dec 15, 2024
Closed
@G-Rath G-Rath mentioned this issue Dec 18, 2024
Closed
@JoshuaKGoldberg JoshuaKGoldberg removed their assignment Feb 4, 2025
@JoshuaKGoldberg
Copy link
Member Author

I'm not confident I'll have time for this soon, so unassigning myself. Still thinking a team member should tackle it though.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Apr 7, 2025
Open
4 tasks
@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented May 6, 2025 •
edited
Loading

Ok I've tried prototyping this locally three separate times. Each time I've ended up having to make >=3 (!) new packages. Essentially, #10383 (comment). @bradzacher I'm sorry for not understanding at first why your comment was 100% on point!

Just noting examples some of the bigger dependency ties:

  • RuleCreator relies on RuleContext and other ts-eslint/Rule portions, currently in utils
  • RuleContext relies on...
    • FlatConfig.LanguageOptions, Linter.ParserOptions, and other ESLint-oriented types, currently in utils
    • ParserServices, currently in typescript-estree
      • ParserServices also relies on TSESTreeToTSNode and other types from typescript-estree
    • TSESTree.Node, currently in types

Prototypes I'd tried out included making a parser-services package too.

@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented May 6, 2025 •
edited
Loading

Actually, more on that - I suspect we might need to split up typescript-estree. Right now it contains:

  • AST node conversion logic & associated map types
  • The ParserServices type
  • createParseSettings, which relies on createProjectService

RuleCreator needs to know AST maps and the ParserServices type, but doesn't need the createParseSettings -> createProjectService pipeline.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue May 6, 2025
Open
4 tasks
@bradzacher
Copy link
Member
bradzacher commented May 6, 2025 •
edited
Loading

spaghetti code

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@bradzacher
Copy link
Member

Part of the reason I originally created /types was to allow us to decouple some of the packages. It allowed me to break a few cycles and stop depending on runtime packages in a few places.

We can move some of the types across to there if we want -- we just need to do be weary of what's moving across (eg we don't want to add typescript as a dep to that package).

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue May 12, 2025
Draft
@JoshuaKGoldberg
Copy link
Member Author

I drafted up an "ideal world" prototype in #11207. It has a few split-outs, in order:

  1. 5378779: splitting a parser-services package out from typescript-estree. This leaves typescript-estree with just AST conversion & types, allowing...
  2. e44bcfc: a types-eslint package for TSESLint.* types as used in both eslint-plugin and parser-services
  3. 90c32b3: finally, a rule-creator package that relies on types-eslint (so, transitively on typescript-estree and not parser-services)

To summarize the draft's dependency shakeups, the createParserServices runtime logic:

  • Is included in packages: parser-services -> utils -> parser
  • Is not included in packages: types & visitor-keys -> types-eslint -> typescript-estree

Problem: removing exports such as createParserServices from typescript-estree would be a pretty huge breaking change.

I'm also not convinced there needs to be a specific parser-services package. The existing parser package right now is a very small, thin layer atop typescript-estree's functions such as createParserServices. I think it would be gentler for users if we moved those functions to parser.

Proposal: How about we do the following non-breaking changes now...

  1. For typescript-estree's logic:
    1. Re-export createParserServices and other TypeScript-program/project-related functions from the parser package
    2. Mark using those functions from typescript-estree as deprecated & point users to the new ways to import them
  2. For TSESLint.* types, effectively do what Brad's comment suggested:
    1. Move what's currently in TSESLint into a new types-eslint package
    2. Re-exports those types from utils in the same TSESLint structure already in use
  3. For RuleCreator:
    1. Move the current RuleCreator definition into a new rule-creator package
    2. Re-export it from utils

...and then in some future major version:

  1. Move createParserServices & co. from typescript-estree to parser
  2. Update docs somewhere to note that pulling RuleCreator from the dedicated rule-creator package can save on dependency tree size for this issue's use cases

@bradzacher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request team assigned A member of the typescript-eslint team should work on this.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rubiesonthesky @G-Rath @JoshuaKGoldberg @bradzacher @ArnaudBarre @kirkwaiblinger
0