raineorshine / npm-check-updates

Find newer versions of package dependencies than what your package.json allows
Other
9.38k stars 325 forks source link

Bad export of generated JavaScript file #1404

Open regseb opened 5 months ago

regseb commented 5 months ago

Steps to Reproduce

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.


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:

raineorshine commented 5 months ago

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 commented 5 months ago

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

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

export default { run }

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

import ncu from "npm-check-updates";

ncu.run({});
raineorshine commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

regseb commented 4 months ago

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.

  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;
  4. cat ncu.d.ts
    declare function run(): string;
    declare namespace run {
      var run: typeof import("./ncu");
    }
    export = run;
  5. node index.mjs
    foo
    foo
  6. node index.cjs
    foo
    foo
  7. npx tsc --esModuleInterop index.ts
  8. 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());
  9. node index.js
    foo
    foo
  10. npx tsc --checkJs --noEmit --esModuleInterop index.mjs

    No errors

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

    No errors

raineorshine commented 4 months ago

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

regseb commented 4 months ago

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. https://github.com/vitejs/vite/issues/2679#issuecomment-994772493

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 commented 4 months ago

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.