Transforming ImportDeclaration or ExportDeclaration causes type specifiers to be output in js files

[nonara]

Type imports/exports which are regularly excluded in compiled JS output get included when updateImportDeclaration / updateExportDeclaration are called during transform.

TypeScript Version: 3+

Related Issues:
#31446 (this is the same issue, but it was closed prematurely by the OP)

The aforementioned issue details the problem well. Unfortunately, the OP closed it before it could be investigated because of finding a workaround. The workaround, however, was an internal method (ts.updateNode) which has been removed in TS 4.0+.

Playground Link:
Reproduction: https://repl.it/@nonara/ts-bug#index.ts

Search Terms: updateImportDeclaration, createImportDeclaration

Code:

a.ts

export type A = string;

index.ts

import { A } from './a'
export { A } from './a'

transformer.ts

function transformer(ctx: ts.TransformationContext) {
  const { factory } = ctx;
  return (s: ts.SourceFile) => ts.visitEachChild(s, visitor, ctx);

  function visitor(node: ts.Node): ts.Node {
    if (ts.isImportDeclaration(node) && ts.isStringLiteral(node.moduleSpecifier))
      return factory.updateImportDeclaration(
        node,
        node.decorators,
        node.modifiers,
        node.importClause,
        ts.createStringLiteral(node.moduleSpecifier.text)
      );

    if (ts.isExportDeclaration(node) && node.moduleSpecifier && ts.isStringLiteral(node.moduleSpecifier))
      return factory.updateExportDeclaration(
        node,
        node.decorators,
        node.modifiers,
        node.isTypeOnly,
        node.exportClause,
        ts.createStringLiteral(node.moduleSpecifier.text)
      );

    return node;
  }
}

Expected Behaviour: Should match output without transformer...

index.js

export {};

Actual Behaviour: (Output with transformer)

index.js

import { A } from "./a";
export { A } from "./a";

[andrewbranch]

@rbuckton can confirm, but I’m not 100% sure this is a bug—if you modify the declaration, how do we know it can still be elided in emit?

[nonara]

@andrewbranch That's a good point. I should note that it also happens if I replace just the moduleSpecifier.text (StringLiteral) node during visit instead of the parent ImportDeclaration.

The use case is the plugin https://github.com/LeDDGroup/typescript-transform-paths - We're transforming for paths.

Perhaps it would make sense to refresh to the type information for a transformed node if the module specifier has been altered and retain it otherwise. Otherwise, any modification of the node (comments, etc) would likely also break elision functionality.

[nonara]

@andrewbranch
Out of curiosity, where in the code would I be able to find the type elision behaviour? I've tried several workarounds in our library, but there are still edge cases in which it fails.

If I had a little detail on where to look in the TS base, I may be able to find time to do a PR before the team gets to it.

cc @sandersn @rbuckton

[andrewbranch]

Calls of isReferencedAliasDeclaration in the transformers:

TypeScript/src/compiler/transformers/ts.ts

Lines 2844 to 2845 in 54f8d38

	// Elide the import clause if we elide both its name and its named bindings.
	const name = resolver.isReferencedAliasDeclaration(node) ? node.name : undefined;

[nonara]

Thanks, @andrewbranch !

I was able to track the behaviour to this:

TypeScript/src/compiler/transformers/ts.ts

Lines 242 to 268 in 54f8d38

	function visitElidableStatement(node: ImportDeclaration | ImportEqualsDeclaration | ExportAssignment | ExportDeclaration): VisitResult<Node> {
	const parsed = getParseTreeNode(node);
	if (parsed !== node) {
	// If the node has been transformed by a `before` transformer, perform no ellision on it
	// As the type information we would attempt to lookup to perform ellision is potentially unavailable for the synthesized nodes
	// We do not reuse `visitorWorker`, as the ellidable statement syntax kinds are technically unrecognized by the switch-case in `visitTypeScript`,
	// and will trigger debug failures when debug verbosity is turned up
	if (node.transformFlags & TransformFlags.ContainsTypeScript) {
	// This node contains TypeScript, so we should visit its children.
	return visitEachChild(node, visitor, context);
	}
	// Otherwise, we can just return the node
	return node;
	}
	switch (node.kind) {
	case SyntaxKind.ImportDeclaration:
	return visitImportDeclaration(node);
	case SyntaxKind.ImportEqualsDeclaration:
	return visitImportEqualsDeclaration(node);
	case SyntaxKind.ExportAssignment:
	return visitExportAssignment(node);
	case SyntaxKind.ExportDeclaration:
	return visitExportDeclaration(node);
	default:
	Debug.fail("Unhandled ellided statement");
	}
	}

Is there anyone who can speak to the comment from 245-249 in more detail? It's looking like in order to properly fix this, we'll need to drop the if (parsed !== node) block and add safeguards for unavailable type info in the visit functions seen in the switch (node.kind) block.

Any detail on the sorts of conditions which would produce the unavailable type info would be helpful! Mainly, I just want to ensure I'm covering all the bases.

Update:

I can confirm that removing the if (parsed !== node) block restores elision. Not immediately seeing that any safeguards would need to be added. Is there actual error potential here?

[andrewbranch]

This section of code is not too surprising, and it further reinforces my impression that while you have a good reason to want the behavior you want, the behavior you’re getting is working as we expect—basically, that you’re asking for a feature, not describing a bug. My guess is that if you add or replace anything material in the declaration (e.g. changing the module specifier (edit: I guess you tried this and it worked), adding or replacing import specifiers), we would have problems with the rest of the logic here. The only thing I can think of that would probably be fine to transform and then process as usual is dropping import specifiers. In your case, since you want to change the module specifier, I think it’s unclear what we should do here. Should we resolve type info of the import specifiers as if you haven’t changed the path?

Suppose you transform import { Foo } from "./a" to import { Foo } from "./b". One resolves to a type, while the other resolves to a value, and there is additionally a global class Foo. Changing the module specifier could change whether the Foo alias appears to be referenced or not. You might say, “Well, because we never ran the checker with this declaration referencing module "./b", we can justifiably take it as a technical limitation that we cannot process this declaration as if it references "./b". We should decide whether or not to elide it based on the original node. If the original node would have been elided, we will elide the transformed node. If the original node would have been preserved, we will emit the transformed node.” (If I understand correctly, this is exactly the behavior you’re looking for.)

But now suppose you have an idea for another transformer. You’re building a dev tool that allows you to visualize the dependency graph of all your modules, as TypeScript sees it (i.e., a dependency on only types from a module still counts, even though it gets elided in JS emit). So, you make a transformer that adds to every module an extra export const __moduleId = "whatever", and adds an import specifier importing that module ID onto every import, so it can call some registration function with it. (This is shaping up to be a gross example, but it’s not really the point.) Now, when you transform import declarations to include, and later use, __moduleId, that is clearly info that you intend to be preserved in the JS emit. But, if we followed the logic from my previous paragraph, we would entirely miss every import that could have been elided based on its original state (i.e., ones that only imported types).

My point is that even if this was very easy to implement safely, it’s still not obviously desirable. You can imagine scenarios where you want to opt into the elision behavior of the original node and scenarios where you don’t. So this still doesn’t seem like something we can just “fix”; I think if you want this behavior, we need to give you some way to signal it, or some way to check whether the original node would have been elided yourself, so you can just return undefined in your own transformer.

But again, @rbuckton (who is on vacation this week) is the expert in this area, so we really need him to weigh in here.

[andrewbranch]

or some way to check whether the original node would have been elided yourself, so you can just return undefined in your own transformer

You actually do have access to this, but it’s an internal API. But if you are just looking to try stuff, you should be able to access context.getEmitResolver().isReferencedAliasDeclaration(node)