Used import elided from emit where source is JavaScript

[wbt]

Bug Report

🔎 Search Terms

import elided javascript used detect unused 6133

🕗 Version & Regression Information

• This is the behavior in every version I tried, and I reviewed the FAQ for entries about elided imports
• I was unable to test this on prior versions because the Playground is only for TypeScript, not JavaScript that's supposed to just be copied through

⏯ Playground Link

Playground link with relevant code

💻 Code

Suppose the following JavaScript calls a function defined in TypeScript:
caller.js:

import * as fs from 'fs';
import TruffleContract from '@truffle/contract'; //Runtime err: this line dropped when JS is copied
import { initContract } from './functions.js';
initContract({}, fs, TruffleContract);
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); //still 'unused'

In functions.ts:

type fsType = typeof import('fs');
type TruffleContractConstructorType = typeof import('@truffle/contract');
import type TruffleContract from '@truffle/contract';
import type { ContractObject } from '@truffle/contract-schema/spec';
export const initContract = function(
    contractJSON: ContractObject | {}, //union type makes example smaller
    TruffleContractConstructor : TruffleContractConstructorType,
    fsDefinition: fsType //for demonstration in calling context
) : TruffleContract.Contract {
    if(typeof TruffleContractConstructor !== 'function') {
        throw('No valid TruffleContractConstructor passed to initContract.');
    }
    return TruffleContractConstructor(contractJSON);
}

A subset of tsConfig.js:

"compilerOptions": {
    "module": "ES2020",
    "moduleResolution": "node",
    "allowJs": true,
    "checkJs": false,
}

package.json has "type":"module" needed for other project requirements, so require is not available. Changing the import syntax to
import { TruffleContract } from '@truffle/contract'; causes TypeScript to maintain the import line but there is an (expected) runtime

SyntaxError: Named export 'TruffleContract' not found. The requested module '@truffle/contract' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from '@truffle/contract';
const { TruffleContract } = pkg;

That again requires manually re-inserting the import statement because TypeScript drops the line falsely asserting that it's not used even when it's used in an expression in the very next line, and it's undefined at runtime because the original syntax is the correct one to use.

🙁 Actual behavior

The TruffleContract import is dropped from the JavaScript file as part of copying that into the target location, leading to a runtime error that TruffleContract is not defined in the expression where it is used. Manually re-adding it to the copied JavaScript file after every transpilation avoids the runtime error.

🙂 Expected behavior

The documentation for allowJs does not give any indication that the TypeScript transpiler will break existing JavaScript, instead of just copying it through. Instead, it says:

This flag can be used as a way to incrementally add TypeScript files into JS projects by allowing the .ts and .tsx files to live along-side existing JavaScript files.

The FAQ states that TypeScript "removes module imports that aren't used in any expression." This one is, so by the reasoning in the FAQ, it isn't being elided, but in practice, it is, breaking what used to be running code.

The workaround discussed there and here is to include a general import line, such as import '@truffle/contract'; and while this does make it through to the target location, it doesn't overcome the runtime error. require is not available for use but the documentation indicates a single use of what's imported should suffice and there is one here.

Overall, the expected behavior is that the JavaScript file is copied directly with no alterations, and certainly no breaking changes.
More specifically, the expected behavior is that an import which is actually used (in this case, it is used for passing a module implementation as a parameter) is not removed. In this case, TypeScript appears to be wrongly detecting that the import is unused (ts(6133)), as the 'unused import' formatting/warning shows up in the editor as well. This incorrect 'unused' status detection even triggers when the imported module is used in e.g. a console.log statement on the very next line, by itself or with the typeof operator.

The implementation is not imported and used directly in functions.ts because different sources of implementations are used at runtime depending on factors which do not appear relevant to a minimal example. This is not in Angular.

[fatcerberus]

I don’t know why TS is eliding the import (that sounds like a bug), but you could try enabling preserveValueImports:
https://www.typescriptlang.org/tsconfig#preserveValueImports

[IllusionMH]

"checkJs": false if you switch it to true you would see Cannot use namespace 'TruffleContract' as a value.

My assumption is that this may be related to incorrect type signature.
Source has module.exports = function () {...} but types only have namespace exported as export default TruffleContract;

I would expect to see export = TruffleContract and declare function TruffleContract(param: type): type in definitions.

[wbt]

So TruffleContract might have written its types badly, but this is working JavaScript code that the TypeScript compiler breaks on copy. TypeScript should not be breaking that, and partial conversion to TypeScript should not require rewriting dependency libraries' types, especially when copying JavaScript seems so simple for TS to handle!

[wbt]

@fatcerberus thanks for the intended-helpful suggestion to try enabling preserveValueImports.
Unfortunately, setting this to true and/or setting "importsNotUsedAsValues" to "preserve" does not lead to the import statement actually being kept in the "copied" JavaScript.
When the latter is used, the emitted file does include import '@truffle/contract'; but without the name, the runtime error noted above (in discussion of the workaround under "Expected Behavior") is still observed.

[IllusionMH]

TS must remove all type imports.
In package declarations there are no non-type exports.

Not sure if namespace with only type export inside can be treated type-only, but there are no valid access to that namespace to use it value position.

In your case you need to fill issue against package to fix bundled declarations.
In the meantime you can augment that module to have proper default export with .d.ts (note that depending on runtime config you might need export =)

declare module '@truffle/contract' {
    // or better types instead of unknown 
    export default function(json?: unknown): unknown {}
}

[wbt]

TS must remove all type imports.

I disagree: That may be true in TypeScript files but there should be no sense of "removing type imports" in non-TS files that it's just copying through.
Further, this isn't a type import. It's a value import, and TypeScript should be able to see that the thing being imported is being used as a value. Whether TypeScript thinks that is correct or not, when checkJS is false, it should be passing the import through to the "copied" JavaScript code. There might be all sorts of real type errors in the JavaScript but where the code hasn't been converted, those errors should be allowed to persist, instead of TypeScript thinking it knows better and introducing new errors that break running code.

This helps avoid the status quo situation where introducing TypeScript in one part of a program means rewriting valid JavaScript in different parts of the program and/or having to retype dependency libraries, which is a whole lot of work.

For anybody else who encounters this issue, the workaround is adding a post-TypeScript build step to copy the JavaScript file as-is from the source directory to the target directory.

I still believe this is an issue in TypeScript which should be fixed in TypeScript, to have TypeScript do that copying by itself without an override.

[fatcerberus]

You seem to be under the impression that TS is supposed to be simply copying .js files into the output, but this is not true. It transpiles them, just like it does with .ts files, because it has to in order to support options like target and module. The only difference is that type checking doesn't happen (unless checkJs is enabled).

[wbt]

@fatcerberus The documentation does give the impression that's what it's doing. Certainly there is no suggestion in there that the valid JS files would have to be rewritten or changed in any way to avoid having TypeScript break them.

[IllusionMH]

From #44619 that implemented it (initial post wasn't updated with other updates from comments, however this logic still holds)

It does not prevent the elision of imports that resolve to types or type-only exports, because that would create runtime errors

While I agree that eliding imports in JS "without notice" is unexpected, on the other hand with info available to TS via built-in package declarations (only types in namespace) it's possible that this will lead to runtime error, but without checkJs it's impossible to warn about it.

Either way (whether this will or won't be changed in TS) type declarations in should be fixed, otherwise that will be elided again when .js changed to .ts.

[wbt]

I agree that TruffleContract types should be fixed and accept that a problem will exist when the .js is changed to .ts, but it's a low priority for them at present and the philosophy of allowing incremental conversion should allow leaving this one as .js (but under the same root as .ts files) without introducing new runtime errors.

It does not prevent the elision of imports that resolve to types or type-only exports, because that would create runtime errors

This import is NOT a type-only import! The compiler should be able to recognize that (a) it's being used as a value, whether or not it thinks that's correct, and (b) type-only imports don't happen in non-TS JS files!

The compiler is already doing the check for (a) and incorrectly concludes that it's not being used; that could be fixed. However, implementing (b) would be more robust and might be much simpler. The compiler already knows which files are JS files that it's skipping checks on; it could also skip elision of whatever it thinks might be a type-only import.

[DanielRosenwasser]

There's possibly a crazy design aspect here, but I would never have thought that we would drop a default export used as a value. It seems like an unmistakable bug that we would consult the .d.ts file on if an import is droppable when we've already seen the import used in value-land.

But instead,

import Yadda from "@truffle/contract";
const blah = Yadda;
initContract({}, fs, Yadda);

is producing the following code

const blah = Yadda;
initContract({}, fs, Yadda);
export {};

Thoughts @andrewbranch?

[andrewbranch]

My recollection of stuff getting marked for preservation is that it’s based on usage, not the target symbol’s declaration space, but given the behavior you’re describing, you must be right that it’s going off the .d.ts and thinking it can’t be a value. I agree this should be changed; usage is a more reliable indicator. I will say that it’s a slightly unusual use case to rely on tsc’s JS-to-JS emit while simultaneously ignoring its check errors, which would be telling you that TruffleContract is a type and cannot be used in a value position, but generally we don’t want you to be totally blocked by bad library types, so it does seem bad that you can’t slap a // @ts-ignore on this and move on.

This does bring up kind of an interesting tangent about what we think it means for you to import types, in plain old ESM imports, in JS files. I guess there is nothing stopping you from doing this:

// @Filename: types.ts
export interface Foo {}

// @Filename: index.js
import { Foo } from "./types"

/** @param {Foo} foo */
function takeFoo(foo) {}

We will happily process this without errors, and there’s like, technically nothing wrong with it since we elide the import declaration, but now that I’m thinking about it, this feels really weird. Like it’s very TypeScripty to casually import types in import declarations. Because so much JS is checked with noEmit, it kind of feels like this import should be an error, because this code is super gonna crash if you try to run it as-is. The fact that we clean it up on emit doesn’t hold much weight for me, because by that token we could also let you put type annotations all over your JS and say “hey, it doesn’t matter, we’ll remove it when we emit.” 🤨

[andrewbranch]

In principle I’m not opposed to saying let’s never do any import elision from JS files, but then we’ll find out how many people are relying on this behavior on purpose 😬

[DanielRosenwasser]

This does bring up kind of an interesting tangent about what we think it means for you to import types, in plain old ESM imports, in JS files.

Even more strange is that in a plain JS file we don't seem to elide the import.

[fatcerberus]

Even more strange is that in a plain JS file we don't seem to elide the import.

huh? I thought this whole issue was about imports getting elided in plain JS files…

[andrewbranch]

So I think the playground is overriding module settings for JS files, but it looks like we don’t do import elision if the module is es****, but we do if it’s commonjs

[andrewbranch]

Well, even that is an oversimplification. With --module esnext, any usage in a value position seems to count, like Daniel and I both expected/recalled. If there’s no usage, it still gets elided. This is basically expected behavior, though I think it would be reasonable for it to do zero elision.

With --module commonjs, the import gets elided even with that usage in a value position, which is the initial bug report. I’m guessing this has something to do with the machinery that has to synthesize the const TruffleContract = require("@truffle/contract").default kind of stuff.

[IllusionMH]

It's elided at least for "module": "ES2020" (as in OP) and "target": "es2020", "moduleResolution": "node" in TS 4.6.3 and next

No importsNotUsedAsValues - import removed completely
"importsNotUsedAsValues":"preserve" - import '@truffle/contract'; is left

[andrewbranch]

Still curious what @DanielRosenwasser thinks about the general case of using ESM imports in JS files to import types. Worth opening another issue?

[DanielRosenwasser]

Looks like someone really awesome filed an issue about that back in 2018 #22159

[wbt]

It might be OK to have some difference in handling based on whether checkJs is true (through tsconfig or comment in the file). #22159 is about an absent but expected error when that is set.

When checkJs is false, I expect TypeScript to just be passing plain JS files, including imports, through as-is, no matter how many errors TS thinks the code has. There are many false positive flags raised by TS and many cases where perfectly valid JS has to be rewritten/changed as part of a conversion to .ts (in addition to even more cases requiring rewriting JS that is valid in practice because of the structure of the HTML it's used with, but that's not theoretically guaranteed so null/undefined/instanceof checks have to be added during TS conversion). As a practical matter, enabling the incremental conversion TypeScript claims it seeks to support requires the ability to completely disable TypeScript's buggy checking and code-changing algorithms on the unconverted portion of the codebase, and add it in piece by piece as resources permit.

We should really not have to be seeing 4-digit commit counts or weeks-long efforts for a single PR to be able to get things back into a stable working state from attempting to convert one file without hacky overrides.

[wbt]

So I attempted to convert that file to TS and hit up against this issue again today, with the value import still being elided.
Including the report here to keep this active, as opposed to other bugs that just get closed once it's been a while since the issue report.

[andrewbranch]

We don’t close stale bugs; we auto-close questions / external issues / working as intended, and occasionally we manually close suggestions that we don’t think we’ll take. This is a bug, and it has a milestone, and an assignee, and was discussed at a design meeting recently, and I already fixed the related #22159, so it’s quite active.