Skip to content
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

fix(type-utils): treat custom type roots as external #6870

Merged
merged 5 commits into from Oct 17, 2023

Conversation

RebeccaStevens
Copy link
Contributor

@RebeccaStevens RebeccaStevens commented Apr 8, 2023

PR Checklist

Overview

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @RebeccaStevens!

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.

@netlify
Copy link

netlify bot commented Apr 8, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 189d04a
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/652ede0ff4a9620007242cdb
😎 Deploy Preview https://deploy-preview-6870--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 91 (🔴 down 4 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 your Netlify site configuration.

@RebeccaStevens
Copy link
Contributor Author

@marekdedic

@marekdedic
Copy link
Contributor

Oh, great! I was going to write in #6866 that I'd like to port that logic to PackageSpecifier :D

BTW, there'll be a merge conflict with #6780, but no big deal, they're both tiny changes...

@marekdedic
Copy link
Contributor

Adding a test for this would probably be quite convoluted, right?

@bradzacher bradzacher added the bug Something isn't working label Apr 10, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yip, similar to the other one, seems reasonable but let's test 👍

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 16, 2023
Copy link
Contributor

@marekdedic marekdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is mixing #6861 and #6868 together - I don't know if that's a good thing, a bad thing, or neither :D

@@ -30,4 +30,15 @@ declare module 'typescript' {
*/
intrinsicName?: string;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding our own type declarations for the internal sourceFileToPackageName seems fishy to me, but that's a decision the maintainers need to do.

I presume you are aware of #6861 - however, there has been no response in the corresponding TS issue....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for now I think it's fine. If they change that API we should see breakages in the TS version upgrade PR.

@JoshuaKGoldberg
Copy link
Member

@RebeccaStevens I'm hoping to get through the last bits of v6 work today since we're set to launch on Monday. I think that'll just entail merging v6 into this PR's branch and adding a few unit tests - which I'm fine to do on my own, and plan on doing in a few hours. Please let me know if you already started and I can hold off 😄

@JoshuaKGoldberg
Copy link
Member

Hmm, yeah, these unit tests are tough. I ran out of time tonight. Will try again tomorrow.

@JoshuaKGoldberg
Copy link
Member

😕 I can't figure this out. 9b49480 shows a unit test that I would think should populate program.sourceFileToPackageName, but it's Map(0) {size: 0}. Either I've misunderstood how to use the combination of typeRoots + sourceFileToPackageName in this unit test (more likely), or that currently-internal API isn't made for this use case (also plausible), or there's a bug with sourceFileToPackageName (less likely).

To repro:

  1. Put a breakpoint on https://github.com/JoshuaKGoldberg/typescript-eslint/blob/9b4948056790f764b3864badc57823db82cc528e/packages/type-utils/src/TypeOrValueSpecifier.ts#L166
  2. Open TypeOrValueSpecifier.test.ts
  3. Launch the VS Code debugger for Run currently open type-utils test (F5)
  4. Observe that program.sourceFileToPackageName is empty, even though earlier in the call stack (https://github.com/JoshuaKGoldberg/typescript-eslint/blob/959819186291a93e17e99e52973a68f3a352c2f5/packages/type-utils/tests/TypeOrValueSpecifier.test.ts#L146), type is correctly resolved to export interface Foo

@RebeccaStevens
Copy link
Contributor Author

Yeah, I gave up on the test too. I came to the conclusion that internal test API thingy isn't setup to do these sort of tests. I didn't want to open that can of worms of editing that.

If I remember right, local tests I tried using a separate repo passed. I can't really turn those into unit test though.

@JoshuaKGoldberg JoshuaKGoldberg deleted the branch typescript-eslint:main July 10, 2023 17:52
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jul 10, 2023

This was unintentionally auto-closed when we merged the v6 branch 🙃 it'll be recreated reopened.

@JoshuaKGoldberg JoshuaKGoldberg changed the base branch from v6 to main July 10, 2023 21:11
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a tricky one. All the existing unit tests still pass, and I couldn't figure out how to add unit tests for the new behavior. It's been sitting for a while... let's ship it. 🤘

Sorry for taking so long!

Cartoon animation of a ship on water

@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 17, 2023
@JoshuaKGoldberg JoshuaKGoldberg merged commit b85f744 into typescript-eslint:main Oct 17, 2023
46 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: v6 typeMatchesSpecifier PackageSpecifier should match custom type roots
4 participants