Currently, it doesn't support this:

import{Type}from'./types';exportfunction(arg:Type){/* ... */}

but only things like this:

importtype{Type1}from'./types';import{typeType2}from'./types';import{typeType3asType4}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?

The type checker can determine whether something is in type space and/or value space.

But for that case ofexport 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?

Done

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.tsexporttypeA=number;

// my-package/src/function.tsimporttype{A}from'./types';exportfunctionf():A{return1;}

// my-package/src/index.tsexport{f}from'./function';

Then, in user-land:

// my-code.tsimport{f}from'my-package';letvalue;// 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 😓

On apure scope analysis level, we do know this. TheA identifier infunction.ts is part of what's exported withf().

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, sincetype aliases sometimes become new things and sometimes are just references to existing things.

Does that answer what you're looking for?

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.tsexporttypeA=number;

// my-package/src/f1.tsimporttype{A}from'./types';exportfunctionf1():A{return1;}

// my-package/src/f2.tsimporttype{A}from'./types';exportfunctionf2():A{return2;}

// my-package/src/index.tsexport{f1}from'./f1';export{f2}from'./f2';export*from'./types';

Then, in user-land:

// my-code.tsimport{f1}from'my-package';importtype{A}from'my-package';letvalue: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?

If we don't enforce that aexport function fn(): A; is in a file that also exportsA, what is the point of this rule?

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

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. HenceKnip.

do you wanna loop in the triage team so we can agree on something together?

[Refactor] I don't thinksimpleTraverse is what we should use here. It's tailored to the way it's currently used inpackages/typescript-estree:

• There's no way to break, but it looks like this usage would want to halt traversal after finding aReturnStatement
• It allows configurable selectors with'enter', but this code just usesReturnStatement

I think it'd be better to avoidsimpleTraverse here, and instead do some or all of:

• Look into the existingforEachReturnStatement, maybe it's exactly what you need?
• Failing that, write a dedicated simpler traverser for just this rule that halts onReturnStatements, omitting anything around parent pointers or'enter'
• Reuse some of the design and logic from one of the rules that does similar things:explicit-module-boundary-types,explicit-function-return-type
  • This one I'm low confidence in, but figured I'd mention...

Created a custom traverser, inspired by the originalforEachReturnStatement

[Testing] Nothing fails with theProgram > removed. Either there's a missing test or this is unnecessary.

Added missing tests

[Testing] I'd thinkkeyof and/ortypeof types should be flagged too:

constfruits={apple:true};exportfunctiongetFruit<Keyextendskeyoftypeoffruits>(key:Key,):(typeoffruits)[Key]{returnfruits[key];}

Thoughts?

That's interesting. So we'll need to flag both types and values?

If so - I'll probably need to rework the reporting logic to also support values, and we should probably rename the rule (?)

EDIT:
Not sure that the developers will always want to export the value, since it's being exported by reference, which means external consumers will be able to change the internals of the code.
Maybe that's the case most of the times?

In this case, it's really thekeyof typeof fruits that needs to be exported. So I'd assume the squiggly would be on there:

exportfunctiongetFruit<Keyextendskeyoftypeoffruits>(//                                   ~~~~~~~~~~~~~~~~~~~

This would certainly make it difficult to write an auto-fixer 😅 glad that that's out of scope for this PR.

I think we need to report only thetypeof part, because that's what really blocks the developers from using it, no?

Do you think we should have a different message for this case? Maybe something like "typeof needs to be exported. please extract it to a type alias"?

only thetypeof part, because that's what really blocks the developers from using it, no?

Well, it's certainlybetter...

exporttypeFruits=typeoffruits;exportfunctiongetFruit<KeyextendskeyofFruits>(//                                   ~~~~~~~~~~~~

...but it strikes me as a kind of arbitrary exception to allow akeyof ... to be inline. What would make this so special?

different message for this case

+1, yes!

What would make this so special?

IDK, what's the difference between these two?

exportdeclarefunction(key:keyofItems);exportdeclarefunction({ items}:{items:Items});

In both cases you construct a type inline based on a type reference. So why wouldn't we force every inline type to be extracted to an alias?

What makeskeyof special compared to a regularTSTypeLiteral in this case?

EDIT:
And sorry for all of the notifications you get from my "Done" comments 😅
it helps me keep track of my progress, because I come back to this PR multiple times after switching context

Taking a step back: the core idea of the rule is that you should be able todirectly import anything that's visible as a type from a module. You shouldn't have tokeyof something,something['some-key'],ReturnType<typeof something>, or use any other indirection in the type system.

Looking at those two examples, here's how the rule should squiggly them:

exportdeclarefunction(key:keyofItems);//                           ~~~~~~~~~~~exportdeclarefunction({ items}:{items:Items});//                                 ~~~~~~~~~~~~~~~~

So why wouldn't we force every inline type to be extracted to an alias?

We should.

umm... great
now I'll need to redo a lot of my work 😂

(I didn't understand this until now, which is stupid, because it's literally in the issue)

anyway, Ithink it's gonna simplifya lot of my code