[node] `node:module` cleanup #71303

Renegade334 · 2024-12-01T01:47:25Z

Was found to be structurally quite messy during recent definition updates. This changeset performs the following:

Moves definitions for module-related globals (eg. module, require) and NodeJS-namespaced interfaces (eg. NodeJS.Module) into module.d.ts, which is in keeping with the paradigm of modules defining related globals where possible (eg. url, stream/web, etc.)
- Also deprecates the global-scope NodeModule and NodeRequire aliases, which were used prior to @types/node v13; the modern equivalent is to use the the namespaced NodeJS.Module etc.
- Removes the last couple of internal uses of the old global aliases. This necessitates a couple of cosmetic $ExpectType changes, but is otherwise a fully transparent change.
Moves all top-level definitions to the Module namespace. Top-level node:module exports were previously haphazardly defined as either namespaced declarations or static elements on the Module class; this change doesn't affect the behaviour of any imports from node:module, but does improve readability and maintainability.
Definition fixes:
- NodeJS.Module:
  - module.require is a simple function, rather than a NodeJS.Require with its associated properties.
- Hooks:
  - The globalPreload hook was removed in v22.
  - LoadHookContext.format may be null or undefined (corresponding to ResolveFnOutput.format)
  - LoadFnOutput.source may be undefined if the format does not require a source parameter (eg. builtin modules)
- Source maps:
  - findSourceMaps() has not accepted an error parameter for quite some time.
  - Adds the options parameter to the SourceMap constructor in v20/v22.
  - SourceMap#findEntry() can return an empty object, similar to SourceMap#findOrigin().
- Module.builtinModules is immutable.
- The undocumented Module.runMain() accepts an optional parameter.
Backports [@types/node] correct nextLoad & nextResolve property optionality #71492 to v18/v20.
Focuses the tests, which were something of a mess beforehand.
Reorders definitions to match the order of the node docs, for improved maintainability.

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run pnpm test <package to test>.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/docs/latest/api/module.html
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

typescript-bot · 2024-12-01T03:30:28Z

@Renegade334 Thank you for submitting this PR!

This is a live comment that I will keep updated.

3 packages in this PR

esm — on npm, on unpkg
httptoolkit__esm — on npm, on unpkg
node — on npm, on unpkg

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ A DT maintainer needs to approve changes that affect more than one package

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 71303,
  "author": "Renegade334",
  "headCommitOid": "5bd8f0a1a0464e75a3510f2e18e8ad3eb32f274c",
  "mergeBaseOid": "5ed1eb15657f15183299a903db34140cb5e9f529",
  "lastPushDate": "2024-12-01T01:47:25.000Z",
  "lastActivityDate": "2025-01-23T07:13:58.000Z",
  "mergeOfferDate": "2025-01-23T01:21:01.000Z",
  "mergeRequestDate": "2025-01-23T07:13:58.000Z",
  "mergeRequestUser": "Renegade334",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "esm",
      "kind": "edit",
      "files": [
        {
          "path": "types/esm/esm-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Richienb"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "httptoolkit__esm",
      "kind": "edit",
      "files": [
        {
          "path": "types/httptoolkit__esm/httptoolkit__esm-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "nktnet1",
        "pimterry"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/module.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/globals.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/module.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/module.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/test/globals.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/test/module.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v20/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v20/module.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v20/test/globals.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v20/test/module.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "galkin",
        "parambirs",
        "eps1lon",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2025-01-23T01:20:00.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 2509550229,
  "ciResult": "pass"
}

typescript-bot · 2024-12-01T03:30:29Z

🔔 @Richienb @nktnet1 @pimterry @microsoft @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @galkin @parambirs @eps1lon @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @nodejs @LinusU @wafuwafu13 @mcollina @Semigradsky — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Renegade334 · 2024-12-01T03:19:32Z

types/node/module.d.ts

+        /* eslint-disable @definitelytyped/no-unnecessary-generics */
+        /**
+         * Register a module that exports hooks that customize Node.js module
+         * resolution and loading behavior. See
+         * [Customization hooks](https://nodejs.org/docs/latest-v22.x/api/module.html#customization-hooks).
+         * @since v20.6.0, v18.19.0
+         * @param specifier Customization hooks to be registered; this should be
+         * the same string that would be passed to `import()`, except that if it is
+         * relative, it is resolved relative to `parentURL`.
+         * @param parentURL f you want to resolve `specifier` relative to a base
+         * URL, such as `import.meta.url`, you can pass that URL here.
+         */
+        function register<Data = any>(
+            specifier: string | URL,
+            parentURL?: string | URL,
+            options?: RegisterOptions<Data>,
+        ): void;
+        function register<Data = any>(specifier: string | URL, options?: RegisterOptions<Data>): void;
+        /* eslint-enable @definitelytyped/no-unnecessary-generics */


As a static class method this definition was invisible to @definitelytyped/no-unnecessary-generics, but now pops up on its radar. The signatures themselves have not changed.

This usage verges on "disguised type assertion", but can't be changed without breaking existing usage.

jakebailey · 2024-12-03T15:46:07Z

Also deprecates the global-scope NodeModule and NodeRequire aliases, which were used prior to @types/node v13; the modern equivalent is to use the the namespaced NodeJS.Module etc.

I might be wrong, but isn't NodeRequire pretty commonly used?

Renegade334 · 2024-12-09T09:17:54Z

These are the last of the global-scope module types, the others having been removed when the declarations were moved to the NodeJS namespace in the v13 changeset. Since then, those global types have been redefined as aliases to the NodeJS types for downstream BC; signposting users away from them shouldn't cause any mischief.

typescript-bot · 2024-12-12T06:48:11Z

Re-ping @Richienb, @nktnet1, @pimterry, @microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

typescript-bot · 2024-12-19T06:51:29Z

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @Renegade334.

(Ping @Richienb, @nktnet1, @pimterry, @microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky.)

typescript-bot · 2025-01-03T06:51:37Z

@Renegade334 Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

typescript-bot · 2025-01-03T06:51:37Z

@Renegade334 I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Jan 8th (in a week) if the issues aren't addressed.

typescript-bot · 2025-01-13T23:12:33Z

@Renegade334 The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds that are failing do not end up on the list of PRs for the DT maintainers to review.

- findSourceMaps() does not accept an error parameter - constructor options since v20.5 - SourceMap#findEntry() can return an empty object

types/node/test/globals.ts

typescript-bot · 2025-01-16T19:01:15Z

@jakebailey Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

jakebailey

I'm not sure if there's anything questionable left here, or if you're waiting for some other review, but LGTM

jakebailey · 2025-01-23T01:20:21Z

(Just be prepared to need to revert it if something catastrophic happens...)

Renegade334 · 2025-01-23T07:13:58Z

Don't believe so.

Ready to merge.

hecerinc · 2025-01-23T22:17:34Z

Hello, I believe this PR has introduced an error into our pipeline:

node_modules/@types/node/module.d.ts(494,13): error TS2403: Subsequent variable declarations must have the same type. Variable 'require' must be of type 'NodeRequire', but here has type 'Require'.

Using

node v18.20.4
typescript 4.9.5

rajsite · 2025-01-23T22:32:28Z

Think we are hitting a similar issues with this change:

> tsc -b

../../node_modules/@types/node/module.d.ts:606:13 - error TS2403: Subsequent variable declarations must have the same type.  Variable 'require' must be of type 'NodeRequire', but here has type 'Require'.

606         var require: NodeJS.Require;
                ~~~~~~~

  ../../node_modules/@types/webpack-env/index.d.ts:277:13
    277 declare var require: NodeRequire;
                    ~~~~~~~
    'require' was also declared here.


Found 1 error.

@types/[email protected]
@types/[email protected]

Pin @types/node due to DefinitelyTyped/DefinitelyTyped#71303 (comment)

jakebailey · 2025-01-23T22:40:22Z

Yeah, this is the kind of thing I was worried about when I was talking about require. I think we need to revert that particular part of the code.

It's not clear to me why this didn't fail CI, given webpack-cli dev deps on the node types.

jakebailey · 2025-01-23T22:47:47Z

Actually, this is puzzling; the definition of NodeRequire in webpack-env is:

interface NodeRequire extends NodeJS.Require {}

declare var require: NodeRequire;

I have no idea why this would fail to work.

jakebailey · 2025-01-23T22:57:51Z

I guess we could also update webpack-env to say NodeJS.Require, but it's confusing why this is needed at all. I am curious if anyone with this problem can try hand-editing their d.ts file for webpack-env with that change and see if it works.

hecerinc · 2025-01-23T23:24:54Z

Made the edit manually and build passed :)

jakebailey · 2025-01-23T23:28:42Z

The commit above from ni/nimble@46a55c9 says TS 5.4, though I don't know how to verify that is what actually ran and failed. https://github.com/search?q=repo%3Ani%2Fnimble%20%2F%22typescript%22%2F&type=code seems to imply so.

Renegade334 · 2025-01-24T02:20:49Z

So, the root cause actually appears to be a compiler quirk (bug?) relating to duplicate inheritance from an interface with one or more callable signatures.

The issue is that there ends up being two duplicate interface NodeRequire extends NodeJS.Require {} declarations, one in @types/node and one in @types/webpack-env. This causes the callable signatures to be duplicated on the "child" interface NodeRequire, which makes the compiler complain when checking for identical var types.

A minimal example:

interface A { (): void }
interface B extends A {}
interface B extends A {}
declare var a: A;
declare var a: B;

The shape of B is now not { (): void }, but actually { (): void; (): void }. Although mutually assignable, these types are non-identical, which means that the second var declaration raises an error.

It looks like parcel-env and webpack-env have interface declarations for NodeRequire, and are therefore affected if @types/node is loaded in the same environment. All other NodeRequire references in DT are just consuming the alias rather than redefining it, so are unaffected.

Renegade334 force-pushed the node-module-cleanup branch 2 times, most recently from a168b43 to 0e6854b Compare December 1, 2024 03:12

Renegade334 marked this pull request as ready for review December 1, 2024 03:29

Renegade334 requested review from eps1lon and peterblazejewicz as code owners December 1, 2024 03:29

typescript-bot added Critical package Edits multiple packages labels Dec 1, 2024

Renegade334 commented Dec 1, 2024

View reviewed changes

typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Dec 12, 2024

Renegade334 force-pushed the node-module-cleanup branch from aa8e7bc to 3ad5190 Compare January 3, 2025 16:11

Renegade334 force-pushed the node-module-cleanup branch from 3ad5190 to d0578d4 Compare January 3, 2025 16:14

Renegade334 force-pushed the node-module-cleanup branch from d0578d4 to 35fc681 Compare January 13, 2025 22:56

typescript-bot added The CI failed When GH Actions fails and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Jan 13, 2025

cleanup: source maps

4a16475

- findSourceMaps() does not accept an error parameter - constructor options since v20.5 - SourceMap#findEntry() can return an empty object

Renegade334 added 5 commits January 16, 2025 09:18

cleanup: reorder definitions to match node docs

c697ff0

cleanup: tests

8f00569

update $ExpectType tests in dependent packages

0e2cd21

test presence of named export

56303f7

backport: partial next{Load,Resolve} context parameter

32059d8

Renegade334 force-pushed the node-module-cleanup branch from 35fc681 to 32059d8 Compare January 16, 2025 09:18

typescript-bot added Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. and removed The CI failed When GH Actions fails labels Jan 16, 2025

jakebailey reviewed Jan 16, 2025

View reviewed changes

types/node/test/globals.ts Show resolved Hide resolved

check for global aliases

5bd8f0a

Renegade334 mentioned this pull request Jan 17, 2025

NodeJS RegisterOptions.parentURL should be optional #71672

Closed

16 tasks

jakebailey approved these changes Jan 23, 2025

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Jan 23, 2025

typescript-bot merged commit 862f389 into DefinitelyTyped:master Jan 23, 2025
15 checks passed

Renegade334 deleted the node-module-cleanup branch January 23, 2025 07:58

rajsite added a commit to ni/nimble that referenced this pull request Jan 23, 2025

Rebuild lock file

46a55c9

Pin @types/node due to DefinitelyTyped/DefinitelyTyped#71303 (comment)

Renegade334 mentioned this pull request Jan 24, 2025

parcel-env, webpack-env: use namespaced interfaces for declarations #71739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node] `node:module` cleanup #71303

[node] `node:module` cleanup #71303

Renegade334 commented Dec 1, 2024 •

edited

Loading

typescript-bot commented Dec 1, 2024 •

edited

Loading

typescript-bot commented Dec 1, 2024 •

edited

Loading

Renegade334 Dec 1, 2024

jakebailey commented Dec 3, 2024

Renegade334 commented Dec 9, 2024 •

edited

Loading

typescript-bot commented Dec 12, 2024

typescript-bot commented Dec 19, 2024 •

edited

Loading

typescript-bot commented Jan 3, 2025

typescript-bot commented Jan 3, 2025

typescript-bot commented Jan 13, 2025

typescript-bot commented Jan 16, 2025

jakebailey left a comment

jakebailey commented Jan 23, 2025

Renegade334 commented Jan 23, 2025

hecerinc commented Jan 23, 2025

rajsite commented Jan 23, 2025

jakebailey commented Jan 23, 2025

jakebailey commented Jan 23, 2025 •

edited

Loading

jakebailey commented Jan 23, 2025

hecerinc commented Jan 23, 2025

jakebailey commented Jan 23, 2025

Renegade334 commented Jan 24, 2025

[node] node:module cleanup #71303

[node] node:module cleanup #71303

Conversation

Renegade334 commented Dec 1, 2024 • edited Loading

typescript-bot commented Dec 1, 2024 • edited Loading

3 packages in this PR

Code Reviews

Status

typescript-bot commented Dec 1, 2024 • edited Loading

Renegade334 Dec 1, 2024

Choose a reason for hiding this comment

jakebailey commented Dec 3, 2024

Renegade334 commented Dec 9, 2024 • edited Loading

typescript-bot commented Dec 12, 2024

typescript-bot commented Dec 19, 2024 • edited Loading

typescript-bot commented Jan 3, 2025

typescript-bot commented Jan 3, 2025

typescript-bot commented Jan 13, 2025

typescript-bot commented Jan 16, 2025

jakebailey left a comment

Choose a reason for hiding this comment

jakebailey commented Jan 23, 2025

Renegade334 commented Jan 23, 2025

hecerinc commented Jan 23, 2025

rajsite commented Jan 23, 2025

jakebailey commented Jan 23, 2025

jakebailey commented Jan 23, 2025 • edited Loading

jakebailey commented Jan 23, 2025

hecerinc commented Jan 23, 2025

jakebailey commented Jan 23, 2025

Renegade334 commented Jan 24, 2025

[node] `node:module` cleanup #71303

[node] `node:module` cleanup #71303

Renegade334 commented Dec 1, 2024 •

edited

Loading

typescript-bot commented Dec 1, 2024 •

edited

Loading

typescript-bot commented Dec 1, 2024 •

edited

Loading

Renegade334 commented Dec 9, 2024 •

edited

Loading

typescript-bot commented Dec 19, 2024 •

edited

Loading

jakebailey commented Jan 23, 2025 •

edited

Loading