-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(eslint-plugin): [require-types-exports] add new rule #8443
base: main
Are you sure you want to change the base?
feat(eslint-plugin): [require-types-exports] add new rule #8443
Conversation
Thanks for the PR, @StyleShit! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/eslint-plugin/tests/rules/require-types-exports.test.ts
Outdated
Show resolved
Hide resolved
No idea why the lint/tests are failing, seems to be fine... Am I missing something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
so I got into a rabbit hole here...
to which level should we follow the exported functions?
should we support things like this?
function func1() { /* ... */ }
const func2 = () => { /* ... */ };
export default {
func1,
func2,
func3: () => { /* ... */ },
obj: {
func4: () => { /* ... */ }
}
};
or this?
function func1() { /* ... */ }
const func2 = () => { /* ... */ };
const functions = {
func1,
func2,
func3: () => { /* ... */ },
obj: {
func4: () => { /* ... */ }
}
};
export default functions;
and to which level should we follow the types?
// (-- imagine the same nesting but with types --)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to which level should we follow the exported functions?
IMO if this rule is adhered to, there should never be a case where folks want a type that's not exported. IMO that includes nested functions like (default|functions).obj.func4
:
// source.ts
interface MyType {
apple: boolean;
}
export default {
obj: {
func4: () => {
const value: MyType = { apple: true };
return value;
},
},
};
import functions from "./source.js";
let value; // What type is this?!
value = functions.obj.func4();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started working on this
There is this magical Scope.through
thing that basically saved me hours of work! ✨
StyleShit@1812e37
Is there anything similar for variables, before I start going crazy with those infinite conditions again?
To make it clear - I'm looking for something that extracts the type references from this for example:
type T1 = number;
type T2 = boolean;
export const value: { a: { b: { c: T1 } } } | [string, T2 | null]= {
a: {
b: {
c: 1,
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, is there any utility to run recursively on objects like the one you mentioned above? (functions.obj.func4()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not super practiced with @typescript-eslint/scope-manager
myself 😅 and don't have a great answer for you. Sorry. I'd say your best bet is to play around with the APIs and the scopes on the playground for your snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything similar for variables, before I start going crazy with those infinite conditions again?
I have a stupid idea that works, not sure whether it's acceptable 😂
will probably need a review on that before I use the same approach for the rest:
e57985a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump?
will be glad to get some directions here, I'm lost...
I have an awful WIP code locally that collects all the type references from the Program, puts them in a Set, and tries to guess the connection between them and the functions' scopes they're related to
(hint: it's ugly and doesn't work 😓)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya sorry, was out - when you say "doesn't work", what do you mean?
Re e57985a#diff-c33471d1953c12297755643e4d460779c5a5323f4535a87cba96ff86abeb40d4R140-R142:
typeReferences.forEach(r => { // TODO: Probably not the best way to do it... if (isLocationOverlapping(r.identifier.loc, node.loc)) {
You might be able to check ts.Node
s by referential equality. Like, if r.identifier === node
, or some similar thing that maybe involves a .parent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by "not working" I mean "I can't make it guess properly" haha
regarding the referential equality check - I managed to make it work thanks to your direction, thanks!
anyway, I still can't find a way to get the types used in an exported object properly 😓
import type { A } from './types';
interface Arg1 {
key: number;
}
interface Arg2 {
key: string;
}
interface Arg3 {
key: boolean;
}
type Ret1 = number;
type Ret2 = string;
type Ret3 = boolean;
const func1 = (arg: Arg1): Ret1 => 1;
function func2(arg: Arg2): Ret2 {
return 'apple';
}
type Apple = string;
const apple: Apple = 'apple';
export const functions = {
func1,
func2,
path: {
to: {
func3: (arg: Arg3): Ret3 => true,
apple,
},
},
};
I know I can get the full type as a string using typeToString()
, but is there a way to get it as an object of nodes/types or something? because seems like this might help me figure out which types have been used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll have to do this manually: a function that takes in an AST node, descends through the AST node, and collects all the types (identifiers & object literals). I don't know of any existing utilities to do this. 😞
'ImportDeclaration[importKind="type"] ImportSpecifier, ImportSpecifier[importKind="type"]': | ||
collectImportedTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it doesn't support this:
import { Type } from './types';
export function (arg: Type) { /* ... */ }
but only things like this:
import type { Type1 } from './types';
import { type Type2 } from './types';
import { type Type3 as Type4 } from './types';
because IDK if I can know for sure whether the imported name is a type or not
any idea if this could be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type checker can determine whether something is in type space and/or value space.
But for that case of export function (arg: Type) { /* ... */ }
, do we need to know whether it's type and/or value? I'd say that should be a complaint in the rule no matter what, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,
After thinking about it for a little bit...
What should we do in cases where someone imports a type from another module and uses it in their exported function?
// my-package/src/types.ts
export type A = number;
// my-package/src/function.ts
import type { A } from './types';
export function f(): A {
return 1;
}
// my-package/src/index.ts
export { f } from './function';
Then, in user-land:
// my-code.ts
import { f } from 'my-package';
let value; // What type is it?
value = f();
I mean... on the one hand, we can't know whether the type is exported somehow to the user.
On the other hand, (I think?) we don't want to force everyone to (re)export the types they imported 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a pure scope analysis level, we do know this. The A
identifier in function.ts
is part of what's exported with f()
.
I think it'd be very preferable to stick with the pure scope analysis strategy. Dipping into types land is much harder to work with, since type
aliases sometimes become new things and sometimes are just references to existing things.
Does that answer what you're looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule would need to check: for every identifier in the return type, is that also exported in the current file (module)?
My current logic is:
"Does this type exist in the outside world (whether it's imported/exported in the current file)?"
- If so: Great!
- Otherwise: Report
I think we do, and that that's the goal of this rule!
I don't fully agree
I mean... yeah, that's the goal of the rule, but the type might already be exported through the index file, for example, and we'll force developers to export it multiple times in their code.
Let's take my example above and modify it a little bit:
// my-package/src/types.ts
export type A = number;
// my-package/src/f1.ts
import type { A } from './types';
export function f1(): A {
return 1;
}
// my-package/src/f2.ts
import type { A } from './types';
export function f2(): A {
return 2;
}
// my-package/src/index.ts
export { f1 } from './f1';
export { f2 } from './f2';
export * from './types';
Then, in user-land:
// my-code.ts
import { f1 } from 'my-package';
import type { A } from 'my-package';
let value: A; // I know what the type is! YAY!
value = f1();
This code is totally fine. But with the changes you suggest, it'll have 2 more redundant exports (and it'll increase linearly based on the type usage inside other modules).
Yeah, it probably won't hurt anybody, but it will make the DX awful, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't enforce that a export function fn(): A;
is in a file that also exports A
, what is the point of this rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could say the same about not exporting it from the index, no?
I mean, you'll always have holes in the rule unless you read the whole codebase, starting from the bundler entrypoint(s)
It's a small code change to enforce that imported types are also exported, I'm just trying to understand where the line lies in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For lint rules, the line generally lies with the single file being linted. Because lint rules operate on a single file at a time, that naturally falls out of the architecture.
It's pretty common that lint rules will only be able to catch issues within the small scope of a single rule at a time. For example, no-unused-vars
won't detect things that are exported from a file but never imported. Hence Knip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you wanna loop in the triage team so we can agree on something together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely start! I've wanted this for ages, so very excited to see you working on it 😄.
Leaving a preliminary review as I think it'll need to be expanded to at least variables. Really, anything that can refer to a type: classes, interfaces / type aliases, namespaces, ...
Good luck on your journey. 😜
return typeReference.typeName.name; | ||
} | ||
|
||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the ast spec:
export declare type EntityName = Identifier | ThisExpression | TSQualifiedName;
This'll need to handle this
and namespace types, no?
export class Wrapper {
work(other: this) {}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... IDK why I haven't looked at spec before writing this 😅
Thanks!
` | ||
export function f<T extends ReturnType<() => string>>(arg: T) { | ||
return arg; | ||
} | ||
`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is failing for some reason. Seems like this code resolves to undefined
for ReturnType
:
const symbol = type.aliasSymbol ?? type.getSymbol();
Am I doing something wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a breakpoint on that line of code this in the debugger, I get:
> services.program.getTypeChecker().typeToString(type)
'string'
I'm inferring that the type is the result of evaluating ReturnType<() => string>
, which reduces to string
. Which actually makes sense to me!
...I don't have a clear thought in my mind as to how to solve for this though. If we skip reporting when !symbol
, then test cases like this one fail:
type Arg = number;
const a = function (a: Arg): void {};
export default a;
I wonder if the conclusion we should make is to not use the type checker at all? As in, just rely on the AST / scope analysis? If a type isn't declared or visible from the current file, it's either...
- ...a built-in one
- ...an error type
- ...something globally augmented in
...and for all of those, I think it's fine to not report?
Closes #7670
Dear Reviewer,
Good luck on your journey!
StyleShit.