Skip to content

JSDoc Typedef should pull name from attached property if missing #19983

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

Closed
weswigham opened this issue Nov 13, 2017 · 6 comments
Closed

JSDoc Typedef should pull name from attached property if missing #19983

weswigham opened this issue Nov 13, 2017 · 6 comments
Labels
Bug A bug in TypeScript Domain: JavaScript The issue relates to JavaScript specifically Domain: JSDoc Relates to JSDoc parsing and type generation
Milestone

Comments

@weswigham
Copy link
Member

weswigham commented Nov 13, 2017

TypeScript Version: 2.7.0-dev.201xxxxx

Code

/** @typedef {{ title: string, colors: !Array.<string>, mutable: boolean }} */
ColorPicker.Spectrum.Palette;

// ...later
ColorPicker.Spectrum.PaletteGenerator = class {
  /**
   * @param {function(!ColorPicker.Spectrum.Palette)} callback
   */
  constructor(callback) {
    this._callback = callback;
    /** @type {!Map.<string, number>} */
    this._frequencyMap = new Map();
    var stylesheetPromises = [];
    for (var cssModel of SDK.targetManager.models(SDK.CSSModel)) {
      for (var stylesheet of cssModel.allStyleSheets())
        stylesheetPromises.push(this._processStylesheet(stylesheet));
    }
    Promise.all(stylesheetPromises).catchException(null).then(this._finish.bind(this));
  }
}

Expected behavior:
ColorPicker.Spectrum.Palette is effectively a type alias for the type in the typedef.

Actual behavior:

front_end/color_picker/Spectrum.js(836,77): error TS1003: Identifier expected.

(A parse error on the trailing * of the typedef).

This is used everywhere in the chrome devtools codebase.

@weswigham weswigham added Bug A bug in TypeScript Salsa labels Nov 13, 2017
@mhegazy mhegazy added the Domain: JSDoc Relates to JSDoc parsing and type generation label Nov 13, 2017
@mhegazy mhegazy added this to the TypeScript 2.7 milestone Nov 13, 2017
@mhegazy mhegazy modified the milestones: TypeScript 2.7, TypeScript 2.8 Jan 9, 2018
@mhegazy mhegazy modified the milestones: TypeScript 2.8, TypeScript 2.9 Mar 9, 2018
@mhegazy mhegazy modified the milestones: TypeScript 3.0, Future Jul 2, 2018
@weswigham weswigham added Domain: JavaScript The issue relates to JavaScript specifically and removed Domain: JavaScript The issue relates to JavaScript specifically Salsa labels Nov 29, 2018
@sandersn sandersn removed their assignment Jan 7, 2020
@TimvdLippe
Copy link
Contributor

I just discovered this issue while trying to migrate DevTools to TypeScript. The following piece of code triggers the Variable 'EventTargetEvent' implicitly has an 'any' type. error

/**
 * @typedef {!{data: *}}
 */
export let EventTargetEvent;

Per https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#supported-jsdoc I have tried the following, but Closure does not accept this syntax:

/**
 * @typedef {!{data: *}} EventTargetEvent
 */

Could typedefs use the name of the variable declaration it is attached to if you don't specify your own name?

@sandersn
Copy link
Member

@TimvdLippe what are the intended semantics? Here is what works in Typescript today:

/**
 * @typedef {!{data: number}}
 */
export let EventTargetEvent;

/** @type {EventTargetEvent} */
var x

x.data

Here, data is defined, and of type number. I think the remaining problem is that there is a noImplicitAny error on EventTargetEvent, right? Or is it that EventTargetEvent is later used as a value, and should have the type EventTargetEvent when used as such?

@TimvdLippe
Copy link
Contributor

I think you are correct. I don't see any type issues per-se when we use EventTargetEvent, so it might just be that the noImplicitAny rule could be enhanced for type-defs.

I just tested the following code and it resulted in the error below (which is probably WAI, it is just a bit hard to spot which errors are correct and which aren't, as TS reports quite a lot of errors atm on existing code):

/**
 * @typedef {!{eventTarget: !Common.EventTarget, eventType: (string|symbol), thisObject: (!Object|undefined), listener: function(!Common.Event)}}
 */
export let EventDescriptor;

/** @type {!EventDescriptor} */
var x = {
  eventTarget: 5,
};
Type '{ eventTarget: number; }' is missing the following properties from type '{ eventTarget: any; eventType: string | symbol; thisObject: Object | undefined; listener: (arg0: any) => any; }': eventType, thisObject, listener

Should I open a separate issue for noImplicitAny?

P.S. Thanks for responding so quickly!

@sandersn
Copy link
Member

Yes, please. When designing checkJS, we thought people would not be using noImplicitAny because it would just be a quick // @ts-check at the top of a file or two. Even in the past couple of years, our model has been a project like webpack, which, while written in Javascript, is written with Typescript compilation in mind.

re response time: You are the beneficiary of my new year's resolution to look at my GH notifications instead of shuddering and shying away!

@sandersn
Copy link
Member

This particular issue is fixed now, I believe by @weswigham a few months ago.

@TimvdLippe
Copy link
Contributor

To answer the noImplicitAny question: we want to turn TypeScript strict mode to catch issues that Closure hasn't caught up to this point. noImplicitAny has already caught several issues with missing or plain out wrong types. It is one of the reasons for doing the migration to TypeScript and we are pleased with how thorough it is at finding issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: JavaScript The issue relates to JavaScript specifically Domain: JSDoc Relates to JSDoc parsing and type generation
Projects
None yet
Development

No branches or pull requests

4 participants