[@types/node] correct `ResolveFnOutput.format` (can be intermediary) #71493

JakobJingleheimer · 2024-12-23T22:23:18Z

format can be any string, it does not have to be one of the final values. It was designed that way so it can signal to its partner load hook that the current module is relevant. ex resolve's format can be typescript, and then load checks format === 'typescript to know whether it do anything.

Only LoadFnOutput.format is restricted to ModuleFormat.

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: I am the author.

…y value) `format` can be any string, it does not have to be one of the final values. It was designed that way so it can signal to its partner `load` hook that the current module is relevant. ex resolve's `format` can be `typescript`, and then `load` checks `format === 'typescript` to know whether it do anything. Only `LoadFnOutput.format` is restricted to `ModuleFormat`.

typescript-bot · 2024-12-23T22:23:54Z

@JakobJingleheimer Thank you for submitting this PR!

This is a live comment that I will keep updated.

1 package in this PR

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 failed
🕐 Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 17 days.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 71493,
  "author": "JakobJingleheimer",
  "headCommitOid": "58a5b711ad38c5d03950a26f95583abc53ae5813",
  "mergeBaseOid": "374a4c6dd7415cc64eb30c834f798f6553fefa06",
  "lastPushDate": "2024-12-23T22:23:18.000Z",
  "lastActivityDate": "2025-01-26T18:50:15.000Z",
  "hasMergeConflict": true,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/module.d.ts",
          "kind": "definition"
        }
      ],
      "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": "stale",
      "reviewer": "Renegade334",
      "date": "2025-01-03T00:00:47.000Z",
      "abbrOid": "b03feaa"
    },
    {
      "type": "stale",
      "reviewer": "Semigradsky",
      "date": "2024-12-26T13:03:41.000Z",
      "abbrOid": "100e34e"
    }
  ],
  "mainBotCommentID": 2560377613,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/58a5b711ad38c5d03950a26f95583abc53ae5813/checks?check_suite_id=32653467752"
}

typescript-bot · 2024-12-23T22:23:57Z

Hey @JakobJingleheimer,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

typescript-bot · 2024-12-23T22:23:58Z

🔔 @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.

typescript-bot · 2024-12-27T17:36:36Z

@JakobJingleheimer 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.

JakobJingleheimer · 2024-12-27T17:56:22Z

The test failure for LoadHook is because it's too naive: LoadContext.format can be any value, but LoadFnOutput can be only ModuleFormat.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/test/module.ts#L92-L104

JakobJingleheimer · 2024-12-31T15:06:55Z

@Semigradsky could you please advise?

types/node/module.d.ts

Renegade334 · 2025-01-02T23:46:34Z

types/node/module.d.ts

        interface ResolveFnOutput {
            /**
-             * A hint to the load hook (it might be ignored)
+             * A hint to the load hook (it might be ignored); can be an intermediary value.
             */
-            format?: ModuleFormat | null | undefined;
+            format?: string | ModuleFormat | null | undefined;


Obviously you're the authoritative source, but can this be partnered with an upstream doc patch to make this behaviour clearer? The existing docblock is taken from the upstream documentation in module.md, which states this this value should be a valid module format string.

Ah, true. I usually look at the jsdoc in the code. Yes, I'll update the markdown docs (I'm not sure how they became incorrect—they didn't use to specify that).

Renegade334 · 2025-01-02T23:47:16Z

types/node/module.d.ts

@@ -165,9 +165,9 @@ declare module "module" {
        }
        interface ResolveFnOutput {
            /**
-             * A hint to the load hook (it might be ignored)
+             * A hint to the load hook (it might be ignored); can be an intermediary value.


Not entirely clear to the uninitiated what "intermediary" should mean in this context? YMMV, but I'd say this line could be left as-is.

Renegade334 · 2025-01-02T23:47:46Z

types/node/module.d.ts

             */
-            format: ModuleFormat;
+            format: string | ModuleFormat;


Ditto.

Suggested change

format: string | ModuleFormat;

format: string;

Renegade334 · 2025-01-02T23:50:33Z

types/node/module.d.ts

@@ -205,9 +205,9 @@ declare module "module" {
             */
            conditions: string[];
            /**
-             * The format optionally supplied by the `resolve` hook chain
+             * The format optionally supplied by the `resolve` hook chain (can be an intermediary value).


Renegade334 · 2025-01-03T00:09:35Z

The test failure for LoadHook is because it's too naive: LoadContext.format can be any value, but LoadFnOutput can be only ModuleFormat.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/test/module.ts#L92-L104

This example is derived from the docs example in module.md, which may itself need addressing given what's being discussed here.

Shouldn't be any problem with adjusting it.

    const load: Module.LoadHook = async (url, context, nextLoad) => {
        if (Math.random() > 0.5) {
            return {
                format: "commonjs",
                shortCircuit: true,
                source: "...",
            };
        }

        return nextLoad(url);
    };

Co-authored-by: René <contact.9a5d6388@renegade334.me.uk>

typescript-bot · 2025-01-03T16:42:30Z

@JakobJingleheimer 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.

JakobJingleheimer · 2025-01-03T16:45:28Z

@Renegade334 it would probably make your lives much easier to consume the jsdocs from the node source-code (ex ResolveHook & LoadHook from customization_hooks.js#L28-L34). These are the dogfood we eat, and they are fully TypeScript-compatible. Finding them may be difficult though.

The markdown docs use some extremely limited subset and parser¹, which tie my wrist to my foot in terms of what I can provide. For instance, in the markdown docs, there is no way to specify that the ResolveContext argument passed to nextResolve is actually Partial<ResolveContext>.

¹ I already proposed fixing it, and that's apparently a very political decision.

JakobJingleheimer · 2025-01-03T17:15:57Z

Upstream fix: nodejs/node#56454

typescript-bot · 2025-01-23T12:56:08Z

@JakobJingleheimer 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-26T18:44:51Z

@JakobJingleheimer 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 Feb 2nd (in a week) if the issues aren't addressed.

JakobJingleheimer · 2025-01-26T18:50:15Z

This is not abandoned

JakobJingleheimer requested review from eps1lon and peterblazejewicz as code owners December 23, 2024 22:23

typescript-bot added Critical package Untested Change This PR does not touch tests labels Dec 23, 2024

JakobJingleheimer mentioned this pull request Dec 23, 2024

feat(types): export type declarations nodejs-loaders/nodejs-loaders#57

Merged

Semigradsky approved these changes Dec 26, 2024

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Dec 26, 2024

fixup!: correct LoadContext.format

b03feaa

typescript-bot added The CI failed When GH Actions fails and removed Owner Approved A listed owner of this package signed off on the pull request. labels Dec 27, 2024

Renegade334 reviewed Jan 3, 2025

View reviewed changes

simplify ResolveFnOutput.format

58a5b71

Co-authored-by: René <contact.9a5d6388@renegade334.me.uk>

typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jan 3, 2025

typescript-bot added the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Jan 23, 2025

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jan 26, 2025

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Jan 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[@types/node] correct `ResolveFnOutput.format` (can be intermediary) #71493

[@types/node] correct `ResolveFnOutput.format` (can be intermediary) #71493

JakobJingleheimer commented Dec 23, 2024

typescript-bot commented Dec 23, 2024 •

edited

Loading

typescript-bot commented Dec 23, 2024

typescript-bot commented Dec 23, 2024

typescript-bot commented Dec 27, 2024

JakobJingleheimer commented Dec 27, 2024

JakobJingleheimer commented Dec 31, 2024

Renegade334 Jan 2, 2025

JakobJingleheimer Jan 3, 2025

Renegade334 Jan 2, 2025

Renegade334 Jan 2, 2025

Renegade334 Jan 2, 2025

Renegade334 commented Jan 3, 2025

typescript-bot commented Jan 3, 2025

JakobJingleheimer commented Jan 3, 2025 •

edited

Loading

JakobJingleheimer commented Jan 3, 2025

typescript-bot commented Jan 23, 2025

typescript-bot commented Jan 26, 2025

JakobJingleheimer commented Jan 26, 2025

[@types/node] correct ResolveFnOutput.format (can be intermediary) #71493

Are you sure you want to change the base?

[@types/node] correct ResolveFnOutput.format (can be intermediary) #71493

Conversation

JakobJingleheimer commented Dec 23, 2024

typescript-bot commented Dec 23, 2024 • edited Loading

1 package in this PR

Code Reviews

Status

Inactive

typescript-bot commented Dec 23, 2024

typescript-bot commented Dec 23, 2024

typescript-bot commented Dec 27, 2024

JakobJingleheimer commented Dec 27, 2024

JakobJingleheimer commented Dec 31, 2024

Renegade334 Jan 2, 2025

Choose a reason for hiding this comment

JakobJingleheimer Jan 3, 2025

Choose a reason for hiding this comment

Renegade334 Jan 2, 2025

Choose a reason for hiding this comment

Renegade334 Jan 2, 2025

Choose a reason for hiding this comment

Renegade334 Jan 2, 2025

Choose a reason for hiding this comment

Renegade334 commented Jan 3, 2025

typescript-bot commented Jan 3, 2025

JakobJingleheimer commented Jan 3, 2025 • edited Loading

JakobJingleheimer commented Jan 3, 2025

typescript-bot commented Jan 23, 2025

typescript-bot commented Jan 26, 2025

JakobJingleheimer commented Jan 26, 2025

[@types/node] correct `ResolveFnOutput.format` (can be intermediary) #71493

[@types/node] correct `ResolveFnOutput.format` (can be intermediary) #71493

typescript-bot commented Dec 23, 2024 •

edited

Loading

JakobJingleheimer commented Jan 3, 2025 •

edited

Loading