Thanks for the PR,@dmichon-msft!

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 onhttps://opencollective.com/typescript-eslint.

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	518da6b
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6780416d2feba700083d75bd
😎 Deploy Preview	https://deploy-preview-10561--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 6 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 yourNetlify site configuration.

View yourCI Pipeline Execution ↗ for commit518da6b.

☁️Nx Cloud last updated this comment at2025-01-13 01:30:24 UTC

As shown in the failed tests this doesn't work as written. I'm guessing that what's happening is that you're deferring the addition of the variables to after the scopes are closed - causing dangingling, unresolved references.

When a scope is finalised we close off the references within the scope. This means we attempt to resolve any unresolved references created in the current or child scopes to variables defined in the current scope. If we don't find the right variable the reference is left as unresolved and we continue to the parent scope.

So if you don't declare the implicit globals early enough then they can't be resolved and so the references are left dangling - which then causes theno-undef rule to error on them.

There's a reason this is a draft. One thing I know I'm doing wrong here is that if there two lib records define the nature of an implicit variable differently (type-only vs. value-only vs. value-and-type), one of them wins at effectively random.

As written this theoreticallyshould run before the global scope is closed, but I'll need to dig into it a bit more. I just wanted to put this out there to get some eyes on the concept.

Ah, I just realized that part of the change is missing here. The invocation ofpopulateGlobalsFromLib was supposed to have been moved to betweenthis.visitChildren() andthis.close() inProgram.

Codecov Report

Attention: Patch coverage is90.00000% with7 lines in your changes missing coverage. Please review.

Pleaseupload report for BASE (main@290211b).Learn more about missing BASE report.
Report is 3 commits behind head on main.

@@           Coverage Diff           @@##             main   #10561   +/-   ##=======================================  Coverage        ?   86.95%           =======================================  Files           ?      446             Lines           ?    15571             Branches        ?     4532           =======================================  Hits            ?    13540             Misses          ?     1676             Partials        ?      355

This is really cool, thanks for sending@dmichon-msft! I'd love to dive in soon and see if we can get this shipped. But - we're a little swamped catching up on issues after the holiday & new year break, and this is a lot of very nuanced code in a not-very-often changed area of the project. So it'll probably be a little while till we can take a deep look.

In the meantime, what would help us review more quickly:

• I see there are some"Added ... was not covered by tests" annotations: could you either test those, or leave comments explaining that they are tested but the annotations are inaccurate (e.g. fromRepo: Merge codecov report from ast-spec into typescript-estree's report #6701 or similar)?
• ThepopulateGlobalsFromLib function has gotten huge and IMO much less readable. maybe split out some well-named helper functions to encapsulate more of the used-only-in-one-place logic/variables?
  • Example:entriesForLib could be aconst if the retrieval logic were moved to a separate function
  • Example:implicitVariablesFromLibs's creation & cache checking could be moved to a separate function
• You mentioned local testing: could you post your reproduction(s) and how you got the measurements?

Btw, you don't need to keep merging the latestmain in:https://typescript-eslint.io/contributing/pull-requests#updating-over-time. We don't want to give that burden to contributors. Especially if us taking a while to get to a PR makes it worse!

I'm slicing out the easy perf win (caching ofObject.entries calls) into its own PR that will be much simpler to review, then I'll refactor this a bit and add more testing to the pieces that power it.

I'm slicing out the easy perf win (caching ofObject.entries calls) into its own PR that will be much simpler to review, then I'll refactor this a bit and add more testing to the pieces that power it.

👍 in that case drafting this PR in the meantime. Let me know if it's actually ready for review and I misinterpreted. Exciting stuff! 🙂

👋 Just checking in@dmichon-msft, do you still have time for this very exciting perf work?

The PR with the bigger part of the performance win merged, so this hasn't been high on my priority list. This wouldn't be terribly difficult to get going again, I'd just largely forgotten about it. The new structure should make this pretty simple.