-
Notifications
You must be signed in to change notification settings - Fork 17
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
Emit correct occurrences for destructured objects #185
Conversation
Previously, scip-typescript didn't emit appropriate occurrences for `property` in the destructuring pattern below: ```ts interface Props { property: number } const prop = { property: 42 } const { property } = prop // ^^^^^^^^ before: was a local reference // now: local definition + global reference ``` The solution works similarly to how to handle shorthand property definitions.
export function foo(): Promise<{ member: number }> { | ||
return Promise.resolve({ member: 42 }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when there is a mapped type (https://www.typescriptlang.org/docs/handbook/2/mapped-types.html) involved as part of the definition? A mapped type can produce new object types, so the keys aren't visible in the source code, they're implicit as part of some logic.
E.g. if you have:
type OptionsFlags<Type> = {
[Property in keyof Type]: boolean;
};
type FeatureFlags = {
darkMode: () => void;
};
type FeatureOptions = OptionsFlags<FeatureFlags>;
// implicitly
// type FeatureOptions = {
// darkMode: boolean;
// }
const fo: FeatureOptions = { darkMode: true };
// ^ go to def
Will go-to-def on darkMode
in the object literal take you to FeatureOptions
? Will it take you to FeatureFlags
? Nowhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one. It doesn't work. I added a snapshot test and opened an issue to follow up on this #187
// ^^^^^^^^ definition local 4 | ||
// documentation ```ts\n(parameter) children: ReactNode\n``` | ||
// ^^^^^^^^ reference react-example 1.0.0 src/`LoaderInput.tsx`/Props#children. | ||
// ^^^^^^^^ reference local 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there an extra reference local
here? The loading
field doesn't have this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, opened #188
// documentation ```ts\nfunction forLoopObjectDestructuring(): number\n``` | ||
for (const { a } of [props]) { | ||
// ^ definition local 41 | ||
// documentation ```ts\nvar a: number\n``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being a var
and not a const
/let
is a bug, right? Filed #186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug indeed, thank you for opening that
// ^^^^^^ definition syntax 1.0.0 src/`structural-type.ts`/foo().Promise:typeLiteral0:member. | ||
// documentation ```ts\n(property) member: number\n``` | ||
return Promise.resolve({ member: 42 }) | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es5.d.ts`/Promise# | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.iterable.d.ts`/Promise# | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/Promise. | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.symbol.wellknown.d.ts`/Promise# | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2018.promise.d.ts`/Promise# | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/PromiseConstructor#resolve(). | ||
// ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/PromiseConstructor#resolve(). | ||
// ^^^^^^ definition syntax 1.0.0 src/`structural-type.ts`/member0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two symbols deliberately different/should they be the same or have a reference relationship?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go to definition doesn't with tsserver for this case. I opened #189 because I think we can make it work anyways, and it would be cool to do better than tsserver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for some of the questions I've left inline.
Previously, scip-typescript didn't emit appropriate occurrences for
property
in the destructuring pattern below:The solution works similarly to how to handle shorthand property definitions.
Test plan
See the updated snapshot tests.