feat(41825): JSDoc equivalent of import * by a-tarasyuk · Pull Request #57207 · microsoft/TypeScript (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation62 Commits32 Checks25 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Contributor

@a-tarasyuk a-tarasyuk commented

Jan 28, 2024

•

edited by DanielRosenwasser

JulianCataldo, keller-mark, vanyauhalin, Thaina, mdrobny, barakplasma, azrdn, meduzen, mikicho, madeindjs, and 8 more reacted with thumbs up emoji keller-mark, Thaina, drazisil, Twipped, bajtos, azrdn, mikicho, w3nl, mvsde, r2dassonville, and 3 more reacted with hooray emoji k-yle, mikicho, kamil-pogorzelski-allegro, r2dassonville, jdspugh, and stambolievv reacted with heart emoji

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/159723/artifacts?artifactName=tgz&fileId=D02528D51446E4533EE8387E4D86BA71B3A7D3FC035CD42935DA2197A5F1290D02&fileName=/typescript-5.4.0-insiders.20240131.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-57207-3".;

Something we should test

/**

@importType
as Foo from "foo" */

and

/**

@importType {
A,
B,
C,
} from "foo" */

In our design meeting, I brought up some of the discussion on Twitter around the tag name.

I think the team is up for just calling it @import.

Something that came up was the use of type modifiers on import specifiers.

/**

@import { type Foo } from "foo" */

The preference was to disallow them for now, even though it might make copy/pasting from TS to JS easier. The idea was can always relax that restriction.

Another two things that I do want us to test:

Auto-imports - whether it works, where they get inserted in an import list, etc.
Declaration file generation - precisely how a /** @import */ comment translates into a TS import within a .d.ts file.

As raised above, multi-line should just work:

/**

@import {
A,
B,
C,
} from "foo" */

However, I don't know exactly how this should be handled, and I am curious to hear thoughts from @sandersn (who can also help out with where the leading * logic lives in our JSDoc implementation).

/**

@import
as Foo from "foo" ^ what is this? */

As I write this up, it really makes me realize like the devil is in the details. 😅

As raised above, multi-line should just work:

/**

@import {
A,
B,
C,
} from "foo" */

@DanielRosenwasser I think the scanner can handle this case. However, I think we need to change the naming for this option :).

if (inJSDocType && !asteriskSeen && (tokenFlags & TokenFlags.PrecedingLineBreak)) {
// decoration at the start of a JSDoc comment line
asteriskSeen = true;
continue;
}

Auto-imports - whether it works, where they get inserted in an import list, etc.

@DanielRosenwasser The existing auto-import logic will add @param { import("./a").A } for the following case., Should we consider changing this logic to use @import?

// @Filename: /a.ts export default interface A { }

// @Filename: /test.js /**

@param { A/**/ } a */ export function f(a) { }

/**

@import
as Foo from "foo" ^ what is this? */

@DanielRosenwasser It's a tricky case because the first * in the JSDoc is a decoration. So, if we handle import tag like other tags, the correct way to use multiline import * should be...

/**

@import
- as Foo from "foo" */

Should we consider changing this logic to use @import?

I would say so - however, in the interest of keeping this PR smaller, you might want to do a follow-up PR instead. @andrewbranch might have thoughts here.

Regarding the ambiguity with *, I am okay with either decision. My suggestion is to say that the next token following a JSDoc import has to occur on the same line. @sandersn might have thoughts here.

Declaration file generation - precisely how a /** @import */ comment translates into a TS import within a .d.ts file.

@DanielRosenwasser These imports are currently transformed into ImportTypeNode ({import("./types").A}). Is it better to translate import tags as top-level declarations within the checker?

const result = resolver.getDeclarationStatementsForSourceFile(sourceFile, declarationEmitNodeBuilderFlags, symbolTracker, bundled);

Since these have type-only import semantics in JSDoc, I would have assumed they would translate to type-only import declarations in the .d.ts file. I’m not sure it actually matters though.

We have to remember that JS-based declaration emit works completely differently from TS-based declaration emit—instead of walking the source file, it walks the symbol table and just synthesizes types for stuff. So as far as the declaration emitter is concerned, this feature really changes nothing. It’s just one more way to give visible symbols a type, but the declaration emitter already doesn’t know or care how anything got its type when working from JS.

Choose a reason for hiding this comment

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

Not done reviewing, but I found that I wrote some requests for tests last time I looked at this and forgot to post them.

Regarding the ambiguity with *, I am okay with either decision. My suggestion is to say that the next token following a JSDoc import has to occur on the same line. @sandersn might have thoughts here.

I'm also OK with any decision, and I think strict no-newline one is a good place to start except that it would require special-casing parseTag, which currently uses

const tagName = parseJSDocIdentifierName(/message/ undefined); const indentText = skipWhitespaceOrAsterisk();

So I'd personally leave it as interpreting a leading asterisk as decoration:

/**

@import
{ foo, bar }
from "multiline.js" */

equivalent to import { foo, bar } from 'multiline.js'. I don't think it's worth any additional code. Just test that

/**

@import
- as multiline from 'multiline.js' */

works.

Choose a reason for hiding this comment

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

I think this is fine? @andrewbranch should give the type-only behavior a once-over, though.

I was skimming for a test like this and I didn’t see one, so let me know if I missed it:

// @declaration: true // @emitDeclarationOnly: true // @checkJs: true // @allowJs: true

// @filename: 0.ts export default interface Foo {} export interface I {}

// @filename: 1.js /** @import Foo, { I } from './0' */

/**

@param {Foo} a
@param {I} b */ export function foo(a, b) {}

Is this input syntax valid, and does it emit valid declaration file syntax? The input syntax should be valid in my opinion, but if it wasn’t handled specifically, it may emit invalid syntax on the output:

import type Foo, { I } from './0'; // ^^^^^^^^^^^^^^^ // A type-only import can specify a default import or named bindings, but not both.(1363)

timocov added a commit to timocov/dts-bundle-generator that referenced this pull request

Apr 1, 2024

I love this feature! I've noticed though, that if you import a type with the new syntax, and then only use that type inside of another typedef (used or unused in the js file), you get a ts6133 "is declared but its value is never read." warning, despite the imported type resolving and the new typedef being declared properly. It only counts read state when something is assigned the type directly.

Hrmmm you are right. I'm having a hard time coming up with a minimal reproducible example. I think it may be related to cycles in the type import, but also related to usage of the cycling type. I'll report back if I can pin it down.

My apologies if this has already been discussed, but it seems the new @import syntax doesn't support import attributes, especially the resolution-mode one that enables @typedef to resolve imports correctly regardless of the type of module being used.

For additional details, my codebase is mostly ESM, but some parts still have to be CJS for compatibility reasons with "old" tools. In those CJS parts, I previously imported types from the ESM part like this:

/** @typedef {import('../path/to/some/ESModule.js', { with: { 'resolution-mode': 'import' } }).MyType} MyType */

I have tried migrating to the new @import tag with an equivalent syntax, but it seems the import attribute is being ignored:

/** @import {MyType} from '../path/to/some/ESModule.js' with { 'resolution-mode': 'import' } */

Using the above syntax yields this error message:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("../path/to/some/ESModule.js")' call instead.
To convert this file to an ECMAScript module, change its file extension to '.mjs', or add the field "type": "module" to '/CJSpart/package.json'. ts(1479)

Am I wrong, or is this simply not available yet for @import? Thanks 🙏

@acidoxee Can you create a new issue? I'll take a look at it.

Sure @a-tarasyuk, here you go! #58955
I hope the format of the issue is OK, I couldn't create a TS playground with several files because of #52666.

For the same project as above, I found another problem: tsc doesn't seem to recognize types imported via @import correctly.

Hi guys, not sure if i should simply open an issue instead of writing here but is it me or it's not possible to just import all ?

/** @import * from "./types.ts" */

In general it's not best practice to comment on a closed PR or issue since people stop following as often, and creates noise in the conversation.

To answer your question, you have to create a name for the import:

/** @import * as myTypes from "./types.ts" */ // ^^^^^^^^^^

JavaScript/TypeScript doesn't support an "import all the things from this module into the current scope" sort of import statement.

Alright, i'm really confused, i swear i used to do this all the time before ESM was a thing but maybe i'm wrong...
Maybe i assumed we could do it since we can do it with export 🤷‍♂️
Thanks !

pokey mentioned this pull request

Jul 18, 2024

Does this only work with .js files, doesn't seem to be working with .ts/.d.ts files.

I want to link a type from another type

/** @import {BetterOmit} from "./better-omit" */

/**

Homomorphic version of the built-in Pick type.
@see {@link BetterOmit} */ type BetterPick<T, K extends keyof T> = { [P in keyof T as K & P]: T[P]; };

If not this, is there some other way of achieving this?

@juhort You are trying to import from a ts file ? "./better-omit.ts" ? Just use the native typescript syntax like you're importing a module or types. This feature is only useful if you do hybrid ts in js via jsdoc.

@jbeaudoin-lf Yeah, importing normally works, but I'm not using that type anywhere apart from the JSDoc comment. Does it still make sense to import only for JSDoc, what's the recommended way of doing something like this?

@juhort Just use ts import in a ts file even if you're only using it in the tsdoc.

The goal of this feature is to improve the DX around jsdoc types in js files (@typedef/@import(..) isn't great). I don't see why you would use this in a ts file since you can import types natively.

I have seen some oddness with node native esm, when you @import a .d.ts it actually expects a .js extension, even if there is no associated .js with that declaration file. The use case being: you want to define some types in real ts syntax but then import it into a .js file.