feat(no-navigation-without-resolve): added support for ResolvedPathname types#1321
feat(no-navigation-without-resolve): added support for ResolvedPathname types#1321marekdedic wants to merge 3 commits intosveltejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 76e7148 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
from
0977c4a to
911af1c
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
2 times, most recently
from
6620c3a to
a81abe5
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
2 times, most recently
from
9ad2044 to
51a0858
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
from
51a0858 to
9a7b9dd
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
from
9a7b9dd to
87f712d
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
2 times, most recently
from
99bcaad to
46d02c7
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
2 times, most recently
from
9c30bbf to
ec4ffa8
Compare
Try the Instant Preview in Online PlaygroundInstall the Instant Preview to Your LocalPublished Instant Preview Packages:
|
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
13 times, most recently
from
e157249 to
09704e9
Compare
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
3 times, most recently
from
f4caee1 to
cdf409b
Compare
|
Hello, I think this one is ready for review. I think it should help a large part of users of the rule as it basically "hides" some other issues (which should be fixed nevertheless, but in the meantime would not impact TS users). Given that I needed to finagle the tests to get this to work, I'd appreciate someone testing it on their own, real-world project. Tagging those who reacted to the PR, maybe you'd have the time to test it out? @shyakadavis @aladh @enchart @lishaduck |
marekdedic
force-pushed
the
no-navigation-without-resove-ResolvedPathname
branch
from
cdf409b to
76e7148
Compare
|
The projects I was working on certainly weren't real world scale and I'm not working on them actively anymore, but just spun up an old project and this seems to be an improvement in it (although, props to the team, it took me some work to find false-positives as is; much improved from when I first set it up). ❤️ |
ota-meshi
left a comment
ota-meshi
left a comment
There was a problem hiding this comment.
Thanks for the PR!
It looks mostly good to me, but I have one question.
| return false; | ||
| } | ||
| const checker = tsTools.service.program.getTypeChecker() as TypeChecker & { | ||
| isTypeAssignableTo(source: Type, target: Type): boolean; |
There was a problem hiding this comment.
Why is this additional declaration necessary? Doesn't TypeChecker already have it?
There was a problem hiding this comment.
This function is a non-public part of the API and therefore not in the types. Nonetheless, it's used by e.g. typescript-eslint. There's an open issue (by typescript-eslint) in TypeScript to make this function part of the public API, so we may be able to remove this assertion once that lands.
There was a problem hiding this comment.
Is it really non-public? Could you please confirm?
It appears to be a public API on my local machine. Am I doing something wrong?
There was a problem hiding this comment.
Hmm, strange, I'll take a look in the evening.
3 participants
Closes #1319
Closes #1320
Closes #1370
Closes #1450