DefinitelyTyped icon indicating copy to clipboard operation
DefinitelyTyped copied to clipboard

Update node:cluster module type def to support CommonJS

Open ggoodman opened this issue 3 years ago • 3 comments

Please fill in this template.

Select one of these and delete the others:

If changing an existing definition:

  • [ ] Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • [ ] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

This PR replaces #59058.

Description

The current type definitions for Nodes cluster module are currently incompatible with CommonJS. In other words, the current type definitions will mislead consumers into thinking that they need to consume the default export even though no such export exists.

This PR changes the module's signature to use the exports= syntax consistent with how the events module is structured.

Status quo

Node.js "type": "module

import * as cluster from 'cluster';

if (cluster.isPrimary) {
  cluster.fork();
} else {
  console.log('Fork succeeded');
}

✅ works

import cluster from 'cluster';

if (cluster.isPrimary) {
  cluster.fork();
} else {
  console.log('Fork succeeded');
}

✅ works

Transpiled to JS using tsc without esModuleInterop

{
  "compilerOptions": {
    "module": "CommonJS"
  },
  "include": ["src"]
}
// src/index.ts
import cluster from 'cluster';

if (cluster.isPrimary) {
  cluster.fork();
} else {
  console.log('Fork succeeded');
}

Produces:

"use strict";
exports.__esModule = true;
var cluster_1 = require("cluster");
if (cluster_1["default"].isPrimary) {
    cluster_1["default"].fork();
}
else {
    console.log('Fork succeeded');
}

❌ Does not work

// src/index.ts
import * as cluster from 'cluster';

//@ts-ignore
if (cluster.isPrimary) {
  //@ts-ignore
  cluster.fork();
} else {
  console.log('Fork succeeded');
}

Produces:

"use strict";
exports.__esModule = true;
var cluster = require("cluster");
//@ts-ignore
if (cluster.isPrimary) {
    //@ts-ignore
    cluster.fork();
}
else {
    console.log('Fork succeeded');
}

✅ Works but does not type check without the //@ts-ignore annotations

However, enabling "esModuleInterop" allows this to work once again.

My conclusion is that the status quo either:

  1. Requires users to enable esModuleInterop; or
  2. Write code that type checks and transpiles but will fail at runtime; or
  3. Write code that doesn't type check but works.

I think that this PR addresses those concerns but am by no means an expert in the nuance of definition file authoring.

ggoodman avatar May 26 '22 15:05 ggoodman

whats the status of this?

daniel-white avatar Sep 29 '22 19:09 daniel-white

Can we please merge this? I face the same issue and not able to use cluster module without any hacks. At least the core modules should be clean enough to use without hacks like ts-ignore or enabling esModuleInterop.

cc @ggoodman is this PR incomplete? if not can you please mark it as ready for review?

sammacorpy avatar Jun 29 '23 06:06 sammacorpy

Can we have this merged? It still does not support CommonJS in 2024.😅

likecyber avatar Jun 13 '24 09:06 likecyber