importsNotUsedAsValues: preserve is not preserving

[robpalme]

Bug Report

My interpretation of the "importsNotUsedAsValues": preserve option is that the goal is to emit value imports as-is. So that you can rely on the JS emit reflecting the JS you write. Here is a case that seems to violate that.

🔎 Search Terms

importsNotUsedAsValues preserve import binding missing skipped omitted import for side-effects JS+types

🕗 Version & Regression Information

• This is the behavior in every version I tried

⏯ Playground Link

Playground link with relevant code

💻 Code

// @importsNotUsedAsValues: preserve

// @filename: child.ts
export default 1

// @filename: main.ts
import d from './child'
eval("d");

🙁 Actual behavior

JS emit for main.ts is missing the import binding.

import './child';
eval("d");

🙂 Expected behavior

The value import should be preserved.

import d from './child';
eval("d");

[andrewbranch]

Hmm, it was an intentional decision that --importsNotUsedAsValues=preserve wouldn’t change the behavior of eliding the actual imported names; it would just keep the module dependency graph as you wrote it. Are there any use cases for this besides stringified execution?

[robpalme]

Ok, I think that's fair.

I was hoping this feature would achieve preservation of the original JS code, i.e. avoiding dead identifier elimination. But I see it does not have that ambition so this is not a bug and I will close it. And having written all this up, I'm now thinking that was an unreasonable assumption on my part 😉

As background, this issue was detected because we ban the import "thing" form (to discourage relying on side-effects) in a build rule that operates after TS->JS conversion - and that was erroring despite the source code not using that form - but I view that as an "us" issue and not convincing general argument. I was also trying to test some name-shadowing behavior by disabling the type-checker but was thwarted - again this is not a practical use-case.

Answering your question, I think stringified execution is the only practical case. In this case, it seems there is no way to get TypeScript to emit the legitimate input JS code even if you disable the checker. So instead, when you detect in your tests or runtime execution that the code is faulty, the workaround is to add a redundant usage site to prevent the compiler from eliminating it.

IMO this is a super-mild case of breaking the JS+Types model because we can't passthrough plain JS. It's an unavoidable transform. Maybe it's also a category mismatch because TypeScript is kinda doing the work of a minifier. Other simplistic forms of dead code, such as unused var declarations, are not eliminated. I appreciate that eliminating type names from the import statements is essential core functionality, and that "not statically detectable as a used value" is a neat heuristic/approximation for achieving that. Fixing that whilst preserving isolated 1:1 per-file transpilation seems to require more thought. I think this is worth thinking about for the future but it's not an urgent matter.

[andrewbranch]

So this same thing came up in the context of Svelte’s transpilation process—they had to write a custom transformer to re-add back imports that were removed because they look unused, but were in fact used in template code, which TS doesn’t understand. Moving some conversation from #43687 and continuing it here.

In #43687 I proposed a mode where:

1. import type declarations are removed (as they are now)
2. import (without type) declarations are not touched whatsoever, regardless of usage—that is, imported names are not removed.
3. It is an error to import anything that is only a type without using import type. (Contrast to --importsNotUsedAsValues=error, which allows pure types to be imported without import type as long as the same import declaration also imports at least one value.)

because

1. That transformer could be removed.
2. You would no longer need to be careful about combining type and value imports, because it would be enforced that you don’t do it.

That seems much better than the status quo, because it would improve transpilation performance and guard against a pitfall that seems very easy to fall into.

[robpalme]

Thank you for reviving the issue @andrewbranch. I see how Svelte needs this same "just remove the types" mode. I like where this is going.

The mode you proposed above sounds reasonable and will solve the problem. It means the whole import statement is either retained or eliminated based on whether it's a type-only import or not.

---

There is a potential extension to the above mode. Which is to introduce new syntax for explicit type-only identifiers. I read this is already supported by Flow and Hegel but cannot find supporting documentation.

import { a, type b, c, type d } from "module";

I think we would still need your new mode in order to error in the case where a or c have no value export in "module". The benefit of explicit type-only identifiers is ergonomics. It helps keep the code DRY. Otherwise some people may be disappointed to find they must write two independent imports to pull in a value and a type from a module.

Please say if you think this extension is better handled in a separate issue - I would be happy to create it.

[dummdidumm]

Thank you for reconsidering this @andrewbranch ! What you wrote is exactly what would be needed. An illustrative example:

<script lang="ts">
   import { value, valueOnlyUsedInTemplate } from './somewhere';
   import type { aType } from './somewhere';

   const foo: aType = value;
</script>
{foo} {valueOnlyUsedInTemplate}

The preprocessor invokes ts.transpileModule with only the contents of the script tag, which would be

   import { value, valueOnlyUsedInTemplate } from './somewhere';
   import type { aType } from './somewhere';

   const foo: aType = value;

Given the current mechanics of TS transpilation, both valueOnlyUsedInTemplate and aType would be removed because they are not used as a value. In case of valueOnlyUsedInTemplate that's wrong. To work around this, the TS Svelte preprocessor adds a transformation which ensures every value imported through import is preserved - but this is also true for interface imports. As a consequence the developer has to tediously separate type and value imports, because TS will merge type and value imports as soon as one value import is given for the same import location. It therefore would be great to reconsider this feature request which would mean

• a new variant of importsNotUsedAsValues with strict separation of type and value imports
• corresponding TS error if type and value import are mixed
• corresponding autocompletion intellisense which keeps imports separate and switches an import from type to value if the semantics change

I think tackling this is closely related to the intention in #39432 which I left a comment on just now.

[milahu]

edit: seems like i misunderstoood

As a consequence the developer has to tediously separate type and value imports

as

we want to mix types and values like import { SomeType, SomeValue } from './some-where'

supporting mixed imports would be a bit more complex

Moving some conversation from #43687 and continuing it here

let me repeat

2. import (without type) declarations are not touched whatsoever, regardless of usage—that is, imported names are not removed.
3. It is an error to import anything that is only a type without using import type

nah, we want to mix types and values like import { SomeType, SomeValue } from './some-where'

-

imports that were removed because they look unused, but were in fact used in template code, which TS doesn’t understand

even worse: ts.transpileModule has zero access to the context

ts.transpileModule has no access to other files, so we must handle the special case
where we dont know whether its a type-import or value-import
and where type-imports can fail when the *.js file does not exist

my suggestion was to add a (rather opinionated) logic for the importsNotUsedAsValues option
that opinionated logic can be avoided by offering a callback interface, something like

var result = ts.transpileModule(source, {
  fileName: "test.ts",
  compilerOptions: {
    target: "es2015",
    importsNotUsedAsValues: function({ importDeclaration, compilerOptions }) {
      // ...
    },
  }
});

where importDeclaration is an accumulated pseudo AST of all imports from one module
including name.usedAsValue and name.usedAsType (as seen by typescript)

importDeclaration examples

const testCases = [

  {
    _ts: `import defaultRename from './some-where';`,
    importDeclaration : {
      name: { escapedText: "defaultRename" },
      moduleSpecifier: { text: "./some-where" },
      namedBindings: undefined,
    }
  },

  {
    _ts: `import defaultRename, { key1, key2 } from './some-where';`,
    importDeclaration : {
      name: { escapedText: "defaultRename" },
      namedBindings: {
        elements: [
          { name: { escapedText: "key1" } },
          { name: { escapedText: "key2" } },
        ]
      },
      moduleSpecifier: { text: "./some-where" },
    }
  },

  {
    _ts: `import defaultRename, { key1 as key1rename, key2 as key2rename } from './some-where';`,
    importDeclaration : {
      name: { escapedText: "defaultRename" },
      namedBindings: {
        elements: [
          { propertyName: { escapedText: "key1" }, name: { escapedText: "key1rename" } },
          { propertyName: { escapedText: "key2" }, name: { escapedText: "key2rename" } },
        ]
      },
      moduleSpecifier: { text: "./some-where" },
    }
  },

  {
    _ts: `import * as moduleRename, { key1 as key1rename, key2 as key2rename } from './some-where';`,
    importDeclaration : {
      namedBindings: {
        name: { escapedText: "moduleRename" },
        elements: [
          { propertyName: { escapedText: "key1" }, name: { escapedText: "key1rename" } },
          { propertyName: { escapedText: "key2" }, name: { escapedText: "key2rename" } },
        ]
      },
      moduleSpecifier: { text: "./some-where" },
    }
  },

  {
    _ts: `import { someThing } from './some-where';`,
    importDeclaration : {
      moduleSpecifier: { text: "./some-where" },
      namedBindings: {
        elements: [
          { name: { escapedText: "someThing" } },
        ]
      },
    }
  },

  {
    _ts: `import { someType } from './some-where';`,
    importDeclaration : {
      namedBindings: {
        elements: [
          { name: { escapedText: "someType", usedAsType: true } },
        ]
      },
      moduleSpecifier: { text: "./some-where" },
    }
  },

  {
    _ts: `import { someValue } from './some-where';`,
    importDeclaration : {
      namedBindings: {
        elements: [
          { name: { escapedText: "someType", usedAsValue: true } },
        ]
      },
      moduleSpecifier: { text: "./some-where" },
    }
  },

];

the callback function could look like 50 lines monster (live demo)

callback function example

var importsNotUsedAsValuesCallback = function({ importDeclaration, compilerOptions }) {
  const { name, namedBindings, moduleSpecifier } = importDeclaration;
  if (moduleSpecifier.text.endsWith('.d.ts')) return false; // elide full import
  const valueImports = []; const dynamicImports = [];
  if (namedBindings?.elements) {
    // import { elem1 } from './some-where'; // -> name: { escapedText: "key1" }
    // import { elem1 as newname1 } from './some-where'; // -> name: { escapedText: "newname1" }, propertyName: { escapedText: "key1" }
    for (const element of namedBindings.elements) {
      const { name, propertyName } = element;
      //console.log('element = ' + JSON.stringify(element));
      if (name.usedAsType == true) {
        //console.log(`elide type import: ${name.escapedText}`);
        continue; // elide import
      }
      const array = (name.usedAsValue == true)
        ? valueImports // keep import
        : dynamicImports; // type or value, we dont know
      array.push({ dst: name.escapedText, src: propertyName?.escapedText });
    }
  }
  if (name) { // import defaultRename from './some-where'; -> name: { escapedText: "defaultRename" }
    console.log('name = ' + JSON.stringify(name));
    if (name.usedAsType == true) { console.log(`elide full type import: ${name.escapedText}`); return false; } // elide full import
    if (name.usedAsValue == true) return true; // keep full import
    // TODO dynamic import
    dynamicImports.push({ dst: name.escapedText, src: 'default' });
  }
  console.log(`found ${valueImports.length} value imports + ${dynamicImports.length} dynamic imports`);
  if (valueImports.length == 0 && dynamicImports.length == 0) return false; // elide full import
  if (valueImports.length > 0) {
    // the import path exists, no need for dynamic import
    return valueImports; // TS will convert the array to JS import code
  }
  // dynamic import
  const importFullModule = Boolean(namedBindings?.name);
  const importProps = valueImports.concat(dynamicImports);
  const importPropsJs = '{ ' + importProps.map(({ dst, src }) => (src ? `${src}: ${dst}` : dst)).join(', ') + ' }';
  const importDst = importFullModule ? namedBindings.name.escapedText : importPropsJs;
  return `\
    // note: top level await only works in modules
    const ${importDst} = await (async () => {
      try { return await import(${JSON.stringify(moduleSpecifier.text)}); }
      catch (error) { console.warn(error.message); }
      return {};
    })();
    ${
      (importFullModule && importProps.length > 0)
        ? `const ${importPropsJs} = ${importDst};`
        : ''
    }
  `;
};

for unused imports, where TS cannot tell if type or value,
this generates fault-tolerant dynamic imports
so when the import is a type-import and the ./some-where.js file is not found,
we only get a warning, and not a fatal error

import * as moduleRename, { key1 as key1rename, key2 as key2rename } from './some-where';

// ... is transpiled to ...

const moduleRename = await (async () => {
  try { return await import("./some-where"); }
  catch (error) { console.warn(error.message); }
  return {};
})();
const { key1: key1rename, key2: key2rename } = moduleRename;

note: this workaround is only needed for development mode.
in production mode, such dead code (unused IDs) should be auto-removed by the build pipeline

[andrewbranch]

Thinking out loud about @robpalme’s suggestion, which ties into the DX concerns in sveltejs/svelte-preprocess#318 (comment):

I do think adding a type keyword to import specifiers is the only way we could plausibly support mixing type and value imports in the same declaration in a mode like this, though I still have reservations about it. The main reason we didn’t allow them from the start is that they look syntactically ambiguous:

import type foo, { Bar } from './mod'

Does type apply to foo or to the whole import? We actually disallowed this construct because it looks unclear, and to allow us to change what we think it means should we have reason to allow individual imported names to be marked as type in the future. I’m not categorically opposed to reconsidering import { type Bar } from './mod', but would lean toward still disallowing import type with a default and named bindings.

[dummdidumm]

These are valid concerns, I indeed would also be confused about the ambiguity. If you really want to add support, it could maybe tweaked like this:

import { Bar, type baz }, type foo from './mode'; // Bar value import, foo/baz type import

So a default import is a type import inside a mixed import if it's last in the chain.

[andrewbranch]

That’s a step too far away from standard ES syntax. We were comfortable with a type prefix in part because Flow did it first, so TC39 would be unlikely to innovate in a way that conflicts. But in general we avoid messing with expression-level syntax.

[robpalme]

This all makes sense. Do you think we must find a way to support the concise type-only default import in combination with value imports?

It feels kinda unnecessary to me given we can already do (non-concise) rebinding which makes it all unambiguous.

import { type default as foo, Baz } from './mod'

[andrewbranch]

I’ve been thinking about this new mode, which will probably be a new value for --importsNotUsedAsValues. I was thinking of a name like preserve-exact or preserve-all. One question this brings up is whether it should even be allowed with --module targets lower than es2015. It certainly becomes a misnomer to say “preserve-exact my imports” if we’re also being told to transform them into a completely different module system. More to the point, in non-ESM module targets, all imports are essentially “namespace” imports, and individual import specifiers are referenced with property access within the code. To take @robpalme’s original example:

import d from './child';
eval("d");

Suppose you compiled this with tsc --importsNotUsedAsValues preserve-exact --module commonjs. Presumably, you would get

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });

const child_1 = __importDefault(require("./child"));
eval("d");

because any actual trackable usage of d would become child_1.default. So the output you get here is different from what you could get with any other set of options that exists today, but it’s not actually useful unless you knew to expect child_1 in the emit, which we would not encourage.

On the other hand, ever since type-only imports were added, we’ve had a consistent stream of people who are frustrated that they can’t opt into a mode where all types must use a type-only import and never be combined with value imports, which is what we’re now thinking of offering—but as far as I could tell, most of the people who have previously asked for that have been motivated by, well, what I consider to be linty pedantry, not because of any sort of runtime constraint. If we were to require es2015+ for this flag, it would probably rain on the parades of some people who want this separation for purely stylistic/DX reasons.

preserve-exact with --module=commonjs or system or amd would not exactly be harmful, but the emit changes it would supposedly produce would be useless and hard to explain. (And skipping emit changes for those module targets would be similarly hard to explain.) I’m leaning toward requiring es2015+. Thoughts?

[milahu]

a mode where all types must use a type-only import and never be combined with value imports

cleaner than my hacky fault-tolerant-dynamic-imports, i have to admit ; )

mixed imports should be auto-fixable by eslint or IDEs, since they can detect type-imports vs value-imports

[dummdidumm]

I'm also in favor of es2015+ . As you said, this is not to please stylistic preferences but to solve transpilation constraints. And through time this problem will hopefully solve itself when the ecosystem as a whole has moved towards ESM.

[robpalme]

Requiring module >= ES2015 to use the new preserve-all-the-things mode seems reasonable.

The primary use-case here is to minimize source transforms and just erase the type syntax, to cater for code/tools whose runtime behavior is broken by todays dead-identifier transform. I foresee it being used most commonly in toolchains that also use target: ESnext.

There's a quote from Paul Rudd in Forgetting Sarah Marshall that I think applies here: "the less you do, the more you do"

The stylistic/lint use-case is new to me. Personally I feel similar to @dummdidumm and would prioritize functional needs over aesthetics. Implementing this for CommonJs, whilst feasible, feels like unnecessary work to me (and maybe goes against the Paul Rudd principle of "do less"?). If the demand for this feature with non-ESM module targets arises in future, maybe the requesters could create an ESLint rule as @milahu suggests.

[andrewbranch]

I foresee it being used most commonly in toolchains that also use target: ESnext.

One question I had, which may be difficult to answer, is whether people who check with TypeScript but transpile with Babel or something else actually set module in their tsconfig.json at all. They might just set noEmit and then ignore any emit-related settings. I briefly considered allowing es2015+ or noEmit, but found that we didn’t have similar heuristics anywhere else in the compiler. I ultimately decided that anyone in that boat should just go ahead and set module: esnext in their tsconfig, as it does affect the type/grammar checking of a few other things, so leaving it off is really a misconfiguration.

[robpalme]

I can't give a general answer, but in my specific case, in which there is no down-levelling because we use a modern engine, we always use module: esnext.

This is used in both the build tools' main API usage and also in the tsconfig we write to disk to drive VSCode which contains noEmit.