-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Module or import
types
#22592
Module or import
types
#22592
Conversation
…s in a file already in the build
src/compiler/types.ts
Outdated
@@ -1063,6 +1064,16 @@ namespace ts { | |||
| SyntaxKind.NeverKeyword; | |||
} | |||
|
|||
export interface ImportTypeNode extends TypeNode { | |||
kind: SyntaxKind.ImportTypeNode; | |||
isTypeOf?: boolean; |
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 you curious why I didn't modify type queries to allow import type nodes somehow? (Perhaps because you dislike 1-token loohaheads or think this feels inelegant?) Well over here is a drop from awhile ago when I tried implementing this that way before, and it was really not pretty. Specifically, most places expect a type query's exprName
to be an Expression (or specifically an entity name), and some kind of import type node (and it is definitely a type node when it's not inside a type query, making the problem more difficult, since it ends up as both a TypeNode
and an Expression
branded node) is not usually an expression, which breaks expectations everywhere. Wrapping all the new behavior up into one new type (node) kind is the neatest way to handle the new feature, without breaking all existing consumers. 😄
@@ -28,8 +27,6 @@ tests/cases/compiler/f3.ts(13,16): error TS4000: Import declaration 'C' is using | |||
import {B} from "./f2"; | |||
~~~~~~ | |||
!!! error TS2667: Imports are not permitted in module augmentations. Consider moving them to the enclosing external module. | |||
~~~~~~ | |||
!!! error TS2307: Cannot find module './f2'. |
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.
These follow-on augmentation errors are now gone (or they can be). Previously, since the imports weren't in a source file, they'd never get processed and never get their names resolved (so the files they reference never get included). Now they still aren't resolved up-front, but inside the checker we can now resolve the name late, and so it does so (relatively to the containing file). I find this to be a better output for this scenario, but it shouldn't particularly matter since it is an error anyway.
Rather than not having a synthetic default, I think you should always have a synthetic default for type This is just taking the problem with That aside, this arguably opens the possibility of explicit type-only imports, instead of only being able to guess from usage. Say, if TypeScript got a "type destructuring" syntax, the following could become possible: // a.ts
export interface A { a: null }
export interface B { b: undefined }
// index.ts
type { A, B } = import('./a')
const a: A = { a: null }
const b: A|B = a |
src/compiler/diagnosticMessages.json
Outdated
"category": "Error", | ||
"code": 1340 | ||
}, | ||
"Import specifier must be a string literal type, but here is '{0}'.": { |
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.
nit, not sure the type name here gives you much value.. we only allow string literals, not even strings.. so i would just say only string literals are valid import specifiers.
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.
We'll parse any type there to be permissive, though; this is more of a grammar error.
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.
I know. i am just saying whatever {0}
is, the error message is just saying you cannot have it.. in other words, the type name is extraneous
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.
I find including the type in the error message useful as frequently an error only exists because I believe that the type is correct at a given location; so seeing where reality differs from my expectation is useful.
src/compiler/checker.ts
Outdated
const currentSourceFile = getSourceFileOfNode(location); | ||
const resolvedModuleState = getResolvedModule(currentSourceFile, moduleReference); | ||
let resolvedModule: ResolvedModuleFull; | ||
if (currentSourceFile && resolvedModuleState === undefined) { |
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.
not sure i understand why we need to do the resolution on two phases, given that we only support string literals as arguments to import
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.
given that we only support string literals as arguments to import
String literal types. Right now it's not an error to say
type Alias = "module";
const x: import(Alias);
the modules in question just only get loaded if they're already included in the build, otherwise produce an error. (see importTypeNested
test.) I can restrain this further, but I don't much see the point in limiting it? I could also handle unions pretty easily as it turns out, too (by bypassing normal typequery logic I've given myself a place to return union types and do multiple namespace lookups).
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.
As we have chatted offline. i think the real value of such feature is to enable higher order types, since we have decided not to do that, i would say no need for the complexity. I would just remove all the deferred module resolution stuff, and keep it simple.
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.
parsing a type to be future proof is fine. but just make it such that the argument has to be string literal node.
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.
@mhegazy Done. It now issues an error if not a string literal, and the late-resolution code has been removed.
src/compiler/checker.ts
Outdated
const links = getNodeLinks(node); | ||
if (!links.resolvedType) { | ||
if (node.isTypeOf && node.typeArguments) { // Only the non-typeof form can make use of type arguments | ||
error(node, Diagnostics.type_arguments_can_only_be_used_in_a_ts_file); |
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.
that error message does not seem right, you probably want something like typeof can not b sued on a type
or type arguments can not be used on a value
or just type arguments can not be used here
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.
'Doh, I put this one here as a placeholder and forgot to replace it with a new message. Thanks for the catch!
src/compiler/checker.ts
Outdated
if (node.isTypeOf && node.typeArguments) { // Only the non-typeof form can make use of type arguments | ||
error(node, Diagnostics.type_arguments_can_only_be_used_in_a_ts_file); | ||
links.resolvedSymbol = unknownSymbol; | ||
return links.resolvedType = anyType; |
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.
unknowType?
src/compiler/checker.ts
Outdated
if (!argumentType || !(argumentType.flags & TypeFlags.StringLiteral)) { | ||
error(node.argument, Diagnostics.Import_specifier_must_be_a_string_literal_type_but_here_is_0, argumentType ? typeToString(argumentType) : "undefined"); | ||
links.resolvedSymbol = unknownSymbol; | ||
return links.resolvedType = anyType; |
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.
unknownType?
src/compiler/checker.ts
Outdated
const moduleName = (argumentType as StringLiteralType).value; | ||
const innerModuleSymbol = resolveExternalModule(node, moduleName, Diagnostics.Cannot_find_module_0, node, /*isForAugmentation*/ false); | ||
if (!innerModuleSymbol) { | ||
error(node, Diagnostics.Cannot_find_module_0, moduleName); |
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.
should not resolveExternalModule already create an error?
src/compiler/checker.ts
Outdated
if (!innerModuleSymbol) { | ||
error(node, Diagnostics.Cannot_find_module_0, moduleName); | ||
links.resolvedSymbol = unknownSymbol; | ||
return links.resolvedType = anyType; |
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.
unknownType?
src/compiler/checker.ts
Outdated
return links.resolvedType = anyType; | ||
} | ||
const moduleSymbol = resolveExternalModuleSymbol(innerModuleSymbol, /*dontResolveAlias*/ false); | ||
if (!moduleSymbol) { |
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.
how would this fail? should not always return a symbol?
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.
I don't think it can, was probably just being a bit too defensive (the function clearly can return undefined
, but probably only when the input symbol is such).
src/compiler/checker.ts
Outdated
const argumentType = getTypeFromTypeNode(node.argument); | ||
const targetMeaning = node.isTypeOf ? SymbolFlags.Value : SymbolFlags.Type; | ||
// TODO: Future work: support unions/generics/whatever via a deferred import-type | ||
if (!argumentType || !(argumentType.flags & TypeFlags.StringLiteral)) { |
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.
i would just check the node to be a string literal.
src/compiler/checker.ts
Outdated
const nameStack: Identifier[] = []; | ||
let currentNamespace = moduleSymbol; | ||
let current = node.qualifier; | ||
while (true) { |
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.
maybe extract to a recursive function to avoid the complexity.
@weswigham you probably still need to look at the LS for this feature, but we can get this in a subsequent PRs. areas that will need attention:
Finally the babel integration may need some changes as well. check with @andy-ms. |
@@ -284,6 +284,7 @@ namespace ts { | |||
IndexedAccessType, | |||
MappedType, | |||
LiteralType, | |||
ImportTypeNode, |
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.
@weswigham is in intentional that this is the only SyntaxKind
with the suffix Node?
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.
T.T no, I just unconsciously reused the interface name (which always uses Node
) for the syntax kind. Will fix.
Fixes #14844
Here's an example in TS:
And in js (the same, but inside jsdoc):
This is a weeeee bit more than was discussed in the design meeting with respect to resolution, as I don't limit the input for
import
types to string literals directly, but just string literal types (and issue an error if the associated file is not yet in the compilation), so they can be aliased (see the pair ofimportTypeNested
tests for an example of this). The downside is that if you use an alias we no longer pick up the file to be included in the project automatically (we do if the literal is inline); but so long as it's included by other means (references, root files, other imports, whatever) it still works out. I still didn't implement a deferred import type to handle generics, however, since we're happy to put that off for now. I also didn't implement handling for unions, but the path forward for them is relatively straightforward in this implementation, compared to a normal import (specifically, since I never need to expose the intermediate symbols, I can just apply the base operation I do right now for each union member and union the results). It's also worth noting that I could easily apply the more flexible top-level lookup logic to dynamic imports to improve our experience there, too. (IE, if someone in JS goesconst a = "./foo.js"; const val = await import(a);
right nowval
isany
, but we could easily allow that to resolve like one may expect providedfoo.js
is also in the project.)Additionally, during the design meeting, we discussed defaulting the type to behaving in a value-y way... that's just not feasible, as it turn out, since you can have constructs like
export = class Foo {}
, and if it were to default to the value side, there's no way to get the correct instance type from it (which is by and large what you'd prefer to have in these situations). 😄 Most of these accesses are probably for namespaced/exported interfaces/class types, so just usingtypeof
when you need a value-space thing's type seems fine (and keeps the syntax in line with other ways to get a module's shape).Additionally, regarding synthetic defaults: making it behave similar to an
import = require
statement (no synthetic default, but no error on non-es shapes) makes it work quite well. At present I'm not installing a synthetic default (after all, a require import wouldn't), since the import type itself is perfectly happy being whatever non-es type it may have been was declared to be (as it is not an emit position); this should help smooth over the esModuleInterop/non-esModuleInterop declaration cases, since, much like a require import, this will be the same under both. (This is also contrary to what we briefly mentioned in the design meeting last week)