feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #8611
Conversation
|
Thanks for the PR, @jakebailey! 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. |
| } | ||
|
|
||
| for (const variable of scope.variables) { | ||
| if (isExported(variable) && !isExportImportEquals(variable)) { |
There was a problem hiding this comment.
declare namespace Foo {
const x = 1;
export const y = 3;
}
Foo.x;
Foo.y;The export doesn't always restrict other implicit, ambient exports, right?
There was a problem hiding this comment.
Hm, the rules for this are super annoying. What I have currently may just be true only for declaration files, but "ambient" ones with declare (in ts files only?) may have different rules?
There was a problem hiding this comment.
It looks like these rules apply to both ts files and dts files as well
playground? Or did I misunderstand you?
It looks like export = is invalid in a namespace but I think that's the only case that invalidates this logic on declared namespaces.
So I think this entire function can just be updated to also short-circuit on if (namespace.declare) { return namespace_has_export_equals }
There was a problem hiding this comment.
There are more issues than just this one IIRC; I've been so busy with other stuff that I haven't been able to sit down and actually fix this PR, sorry.
I'm also not 100% sure what works and doesn't work in the playground when it comes to d.ts files; not really something we often play with; I've been meaning to figure out something better for testing this
If it's bugging you all to stay open, I can close it, though if someone else attempts it too I'd love to review it and double check everything (but I don't think anyone's working on it besides me)
There was a problem hiding this comment.
Nah Josh just pinged me cos I didn't respond :P
No rush at all.
|
👋 just checking in @jakebailey, are you waiting on us for anything? |
|
I honestly don't remember, I think I was blocked on "man the rules are complicated to implement" and finding time to figure them out to try again here. |
|
👍 should we close this out then? (do you know if you'll have time + priority soon?) |
|
5.5 just closed so I'll probably have more time now, but I guess I don't know why closing it is helpful... Only been 2 months and I only stopped because I had some very high priority work elsewhere. |
|
Or course if someone else wants to attempt it, be my guest, but I do want to see this fixed so I can run it on DT. |
|
Staying open it is! We're just going through the backlog & trying to trim it down as much as we reasonably can. |
|
Sure; I'll go look at that when I have a chance. It'd be nice to be able to enable this rule on DT. |
PR Checklist
Overview
This modifies the unused variable analysis to ignore declarations that are "implicitly" exported. Roughly, this means declaration files which do not contain any exports, recursively into namespaces, excluding
export import ... = ...which doesn't count.This PR is not yet complete; I'm missing:
export {}blocks, including empty ones (which probably don't show up in scope)ambientDeclarationSelectorambientDeclarationSelectorentirely (if not vice versa)I'm mainly having trouble at the moment debugging a single test; adding
onlyto the test doesn't seem to really work and one of my tests fails for a reason I haven't figured out yet.That and my continued issues with☹️
nx, where it always tells me that the tests pass, so I have to disable it, which can only be done via the checked in config file