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: https://github.com/microsoft/TypeScript/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:

https://github.com/microsoft/TypeScript/blob/54f8d38535426814d1c5972e4ec4676e8cfebcf6/src/compiler/transformers/ts.ts#L2844-L2845

[nonara]

Thanks, @andrewbranch !

I was able to track the behaviour to this:

https://github.com/microsoft/TypeScript/blob/54f8d38535426814d1c5972e4ec4676e8cfebcf6/src/compiler/transformers/ts.ts#L242-L268

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)

[nonara]

Yes, that does make perfect sense.

One thing to bear in mind is - if we touch anything on any one of these elidable nodes during transform, all regular elision functionality breaks down, and we have no easy path to remedy that. I think that's a fair representation of the core of the issue.

Again, our use case is transforming for paths and virtual directories. I believe that our library is the most widely used for that purpose. We recently rewrote it to rely entirely on TS API as well as adding virtual dir support. The hope is to keep reliance on the API, even if we need to use a few internal API methods.

[AndyOGo]

The same bug is happening for ts-rename-import-plugin. https://github.com/banyudu/ts-rename-import-plugin

[rbuckton]

I can investigate loosening this behavior in visitElidableStatement. In the meantime I'm curious, is it feasible to implement your transform as an after transformer instead? There are more cases to handle (i.e., commonjs, AMD, SystemJS, etc), but import elision will have already happened by that point.

[nonara]

@rbuckton I considered going that route. It would be a breaking change, and I'm not entirely sure what edge cases may arise, so I was hoping to avoid it. If it turns out that there isn't a good solution on the TS side, I'd consider making the shift in the next major version.

Perhaps it would work to remove the the parsed !== node check in visitElidableStatement and add checks in the visitors, where applicable. As an example, the import specifier visitor could compare the original node to determine elidability on a per-specifier basis.

Continuing the example in practice - say we have 10 import specifiers in an ImportDeclaration. During transform, we remove two, alter one, and add another. In that case — ideally — elision for the altered and the added would be indeterminable, but the remaining would be determinable.

[squidfunk]

Also happening for ts-transform-paths.

EDIT: solved in 2.0.2 by using an after transform instead of before

[uasan]

It fixes

node.moduleSpecifier = context.factory.createStringLiteral(newPath);

sourceFile.resolvedModules.set(
  newPath,
  sourceFile.resolvedModules.get(oldPath)
);

But it hack?