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

JSDoc @type does not attach to for loop variable declarations #43756

Open
Paril opened this issue Apr 21, 2021 · 9 comments
Open

JSDoc @type does not attach to for loop variable declarations #43756

Paril opened this issue Apr 21, 2021 · 9 comments
Assignees
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@Paril
Copy link

Paril commented Apr 21, 2021

Bug Report

From what I could tell, this is a common pattern with JSDoc used to properly document the type of a loop variable. It does not seem to work in VSCode.

🔎 Search Terms

jsdoc for loop type

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about JSDoc.

⏯ Playground Link

https://www.typescriptlang.org/play?#code/GYewTgBAFA9AVHCABALgTwA4FMIG8ASAKgLIAyAogDZYC2WAdigL4RwwQDGI9AzihDhDAIAExAcArnUYA6AEYgRaGQEcJWMGgDKWahxTgAgpUpQA5DJRY+ZgJS28AKAguBMvmmoyRASx4ZKAEM0CABeCDN6biwzAG5HJiA

💻 Code

for (/** @type {HTMLElement} */ const e of document.body.querySelectorAll('.test')) {
    e.style.display = 'none';
}

🙁 Actual behavior

e is typed as Element

🙂 Expected behavior

e should be typed as HTMLElement, as I have overridden it via @type.

As an example, if I were to change the code to this:

/** @type {HTMLElement} */ const e = document.body.querySelectorAll('.test');

VSCode correctly picks up that I'm wishing to override e as an HTMLElement (and complains about missing interface implementations, but, that's not the issue here - in fact that's expected!)

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Apr 21, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.4.0 milestone Apr 21, 2021
@Paril
Copy link
Author

Paril commented Apr 21, 2021

Also, for those who need a workaround, this works using TypeScript's JS "cast" feature, but I find it much harder to work with:

for (const e of /** @type {NodeListOf<HTMLElement>} */ (document.body.querySelectorAll('.test')))

@sandersn
Copy link
Member

@Paril do you have an idea of how common this is?

This is a non-trivial feature (though probably easy-ish) because Typescript explicitly disallows type annotations here. All the type checking would have to be added from scratch.

@sandersn
Copy link
Member

In my usual code bases from Definitely Typed dependencies and our user tests + dependencies, I grepped for.+@type and didn't find any existing uses.
I did find 3 uses of the cast syntax in chrome-devtools-frontend and 1 in webpack, but those projects use Closure or Typescript.

Looking at this strictly as a new feature, I'm inclined to reference Closure and Typescript for precedent and say that allowing annotations here isn't that useful.

@Paril
Copy link
Author

Paril commented Apr 22, 2021

I'm not sure about how common it is, I shouldn't have used the word "common pattern" in the initial post as I'm not actually certain of its usage but it was a pattern I saw as an accepted answer on how to do this sort of thing with JSDoc. It seems like it should be allowed; there's no other way to annotate a variable in a for/foreach statement otherwise, which seems like an odd missing feature.

What's the reason for explicitly disallowing type annotations here? I'm curious as to why variable declarations in other contexts are okay to annotate but loop ones aren't, unless you move the declaration outside of the statement itself. I guess it just felt like it should "just work" because it immediately precedes the declaration.

The two workarounds are much more verbose, and I can't see this being uncommon in the context of trying to use type checking inside of JS since querySelector and friends tend to prefer Element, ChildNode, and all of those non-specific interfaces.

@Paril
Copy link
Author

Paril commented Apr 22, 2021

Actually, I think I found the TypeScript equivalent of this one, and I understand the reasoning for it being not accepted:

#25605
#3500

Given how annotating this inline is essentially the JS version of this issue, I see the precedent for closing this one.

However, the latter issue is still open for discussion; should this just be moved into that discussion perhaps?

@sandersn
Copy link
Member

@Paril we were reading the same thing! This is still a separate issue from #3500, but I'd say it depends on that one. Let's leave this open, and once #3500 is fixed, it will be a small amount of work to make sure it works in JS too.

@Paril
Copy link
Author

Paril commented Apr 22, 2021

Sounds good to me!

@Paril
Copy link
Author

Paril commented Apr 23, 2021

Also, I just realized that this syntax is fully valid in VSCode:

const substring = (/** @type {string} */ str, /** @type {number} */ start, end = str.length) => str.substring(start, end);

This isn't a huge thing, but, I think it's a good example of existing code that is similar to what I'm hoping for (although this is a parameter, not a local variable, but hey, same difference)

@sandersn sandersn added this to Not started in Rolling Work Tracking Jun 28, 2021
@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 30, 2021
@sandersn sandersn added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Oct 28, 2021
@sandersn
Copy link
Member

Changed the labels to match #3500.

@sandersn sandersn removed this from Not started in Rolling Work Tracking Oct 28, 2021
@sandersn sandersn removed this from the TypeScript 4.5.0 milestone Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants