Skip to content
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

Bad export of generated JavaScript file #1404

Open
3 tasks done
regseb opened this issue May 2, 2024 · 11 comments
Open
3 tasks done

Bad export of generated JavaScript file #1404

regseb opened this issue May 2, 2024 · 11 comments
Labels

Comments

@regseb
Copy link
Contributor

regseb commented May 2, 2024


Steps to Reproduce

  • package.json

    {
      "name": "testcase",
      "version": "1.0.0",
      "dependencies": {
        "npm-check-updates": "16.14.20",
        "typescript": "5.4.5"
      }
    }
  • index.ts

    import ncu from "npm-check-updates";
    
    ncu.run({});

Steps:

  1. npm install
  2. npx tsc index.ts

Current Behavior

index.ts:3:5 - error TS2339: Property 'run' does not exist on type '(runOptions?: RunOptions, { cli }?: { cli?: boolean; }) => Promise<void | PackageFile | Index<string>>'.

3 ncu.run({});
      ~~~


Found 1 error in index.ts:3

Expected Behavior

No error.


  • src/index.js

    export async function run(
      runOptions: RunOptions = {},
      { cli }: { cli?: boolean } = {},
    ): Promise<PackageFile | Index<VersionSpec> | void> {
      // ...
    }
    
    export default run
  • build/src/index.js

    async function run(runOptions = {}, { cli } = {}) {
        // ...
    }
    exports.run = run;
    exports.default = run;
  • build/src/index.d.ts

    export declare function run(runOptions?: RunOptions, { cli }?: {
        cli?: boolean;
    }): Promise<PackageFile | Index<VersionSpec> | void>;
    export default run;

The JavaScript file generated isn't good because it's not possible to convert an ESM which has a default export and a named export. A CJS only has a default export (which may be an object to simulate named exports). exports.default = run; exports a property "default" in default object exported.

I think you only need to export a default object with the run property:

  • for JavaScript generation to be correct;

    async function run(runOptions = {}, { cli } = {}) {
        // ...
    }
    exports.run = run;
    // Or:
    async function run(runOptions = {}, { cli } = {}) {
        // ...
    }
    exports = { run };
  • and to don't break backward compatibility.

    import ncu from 'npm-check-updates'
    
    const upgraded = await ncu.run({
      // ...
    })
@raineorshine
Copy link
Owner

raineorshine commented May 3, 2024

Hi, thanks for reporting.

I tried both exports.run = run and exports = { run } without removing the default export, but neither of them worked. I still get Property 'run' does not exist. I don't want to remove export default run since that is the cleaner syntax, but I'm clearly confused about how to do both. If I had to choose, I would be fine with just the default export.

In the mean time, it does seem that the following works in Typescript files using CJS imports:

import ncu from 'npm-check-updates'

const upgraded = await ncu({
  // ...
})

@regseb
Copy link
Contributor Author

regseb commented May 3, 2024

With this src/index.js (TS Playground):

export async function run(
  runOptions: RunOptions = {},
  { cli }: { cli?: boolean } = {},
): Promise<PackageFile | Index<VersionSpec> | void> {
  // ...
}

export default { run }
  • JavaScript generated exports by default : { run, default: { run } }
  • Declaration file generated exports by default : { run }

So, tsc shouldn't report any error for the following code:

import ncu from "npm-check-updates";

ncu.run({});

@raineorshine
Copy link
Owner

Yes, but then ncu({}) would not work, I think? I'd be willing to have a default export at the expense of ncu.run, but not the other way around.

There is a major version coming out soon, so it's okay to be break ncu.run if we have to.

@regseb
Copy link
Contributor Author

regseb commented May 4, 2024

To export by default, use the export = syntax.

async function run(
  runOptions: RunOptions = {},
  { cli }: { cli?: boolean } = {},
): Promise<PackageFile | Index<VersionSpec> | void> {
  // ...
}

export = run

But it will do a breaking change: ncu.run() won't work anymore. The new usage will be:

import ncu from 'npm-check-updates'

const upgraded = await ncu({
  // Pass any cli option
  packageFile: '../package.json',
  upgrade: true,
  // Defaults:
  // jsonUpgraded: true,
  // silent: true,
})

console.log(upgraded) // { "mypackage": "^2.0.0", ... }

@raineorshine raineorshine added this to the v17 milestone May 10, 2024
@raineorshine
Copy link
Owner

raineorshine commented May 10, 2024

The index file also does export type { RunOptions } so unfortunately we can't use the exports = run syntax.

It seems that there isn't a trivial fix here, but I'm definitely interested in switching to a default export if we can figure out how to make it work with both esm and cjs. I'm open to a pull request if you have a better grasp of typescript and esm than me.

In the mean time, it does seem that the following works in Typescript files using CJS imports:

import ncu from 'npm-check-updates'

const upgraded = await ncu({
  // ...
})

Out of curiosity, does this work for you currently?

@regseb
Copy link
Contributor Author

regseb commented May 10, 2024

Out of curiosity, does this work for you currently?

import ncu from 'npm-check-updates'

console.log(ncu);
const upgraded = await ncu({
  // ...
})

Here's the terminal when I run the JavaScript code above:

{ run: [AsyncFunction: run], default: [AsyncFunction: run] }
file:///home/regseb/testcase/index.js:4
const upgraded = await ncu({
                       ^

TypeError: ncu is not a function
    at file:///home/regseb/testcase/index.js:4:24
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5)

@raineorshine
Copy link
Owner

Okay, interesting. So it's just the types that are wrong.

@regseb
Copy link
Contributor Author

regseb commented May 15, 2024

I think it should be fine with the following code:

async function run(
  runOptions: RunOptions = {},
  { cli }: { cli?: boolean } = {},
): Promise<PackageFile | Index<VersionSpec> | void> {
  // ...
}

run.run = run;

export = run

Here are the tests I ran.

  • package.json

    {
      "name": "testcase",
      "version": "1.0.0",
      "dependencies": {
        "npm-check-updates": "16.14.20",
        "typescript": "5.4.5"
      }
    }
  • ncu.ts

    function run() { return "foo"; }
    
    run.run = run;
    
    export = run;
  • index.mjs

    import ncu from "./ncu.js";
    
    console.log(ncu());
    console.log(ncu.run());
  • index.cjs

    const ncu = require("./ncu.js");
    
    console.log(ncu());
    console.log(ncu.run());
  • index.ts

    import ncu from "./ncu";
    
    console.log(ncu());
    console.log(ncu.run());
  1. npm install
  2. npx tsc --declaration ncu.ts
  3. cat ncu.js
"use strict";
function run() { return "foo"; }
run.run = run;
module.exports = run;
  1. cat ncu.d.ts
declare function run(): string;
declare namespace run {
    var run: typeof import("./ncu");
}
export = run;
  1. node index.mjs
foo
foo
  1. node index.cjs
foo
foo
  1. npx tsc --esModuleInterop index.ts
  2. cat index.js
"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
var ncu_1 = __importDefault(require("./ncu"));
console.log((0, ncu_1.default)());
console.log(ncu_1.default.run());
  1. node index.js
foo
foo
  1. npx tsc --checkJs --noEmit --esModuleInterop index.mjs

No errors

  1. ncu npx tsc --checkJs --noEmit --esModuleInterop index.cjs

No errors

@raineorshine
Copy link
Owner

Could you open a PR? That would be a big help.

@regseb
Copy link
Contributor Author

regseb commented May 18, 2024

I tried with export = run

An export assignment cannot be used in a module with other exported elements.

So I deleted export type { RunOptions }

RollupError: src/bin/cli.ts (7:7): "default" is not exported by "src/index.ts", imported by "src/bin/cli.ts".

I've added commonjsOptions: { include: [] }, in Vite config. vitejs/vite#2679 (comment)

RollupError: src/lib/getRepoUrl.ts (2:7): "default" is not exported by "node_modules/hosted-git-info/lib/index.js", imported by "src/lib/getRepoUrl.ts".

I don't think this is possible as long as the TypeScript bug hasn't been fixed.


TypeScript generates inconsistencies between the JavaScript and the declaration file only for CJS files. Another solution would be to generate ESM files. In this TS Playground, JS and .D.TS are consistent. But I don't know about compatibility with older versions of Node.

@raineorshine
Copy link
Owner

Thanks for investigating. I don't have time at the moment to investigate further but I'll keep this issue open in case anyone else can help.

@raineorshine raineorshine removed this from the v17 milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants