[@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 passed
✅ Most recent commit is approved by a DT maintainer

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": 71493,
  "author": "JakobJingleheimer",
  "headCommitOid": "9adb1adbfbf8a57798af14df4b6293e7913111d1",
  "mergeBaseOid": "1880268063b4c3a1e06c661c544790fa312c5120",
  "lastPushDate": "2024-12-23T22:23:18.000Z",
  "lastActivityDate": "2025-03-03T17:40:02.000Z",
  "mergeOfferDate": "2025-03-03T17:38:45.000Z",
  "mergeRequestDate": "2025-03-03T17:40:02.000Z",
  "mergeRequestUser": "JakobJingleheimer",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/module.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/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": "gabritto",
      "date": "2025-03-03T17:38:07.000Z",
      "isMaintainer": true
    },
    {
      "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": "pass"
}

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

        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.

Suggested change

* A hint to the load hook (it might be ignored); can be an intermediary value.

* A hint to the load hook (it might be ignored); can be an arbitrary value to be changed at a later stage (eg `css` → `json`).

@Renegade334 thoughts on this updated comment wording?

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

types/node/module.d.ts

+             * The format optionally supplied by the `resolve` hook chain (can be an intermediary value).
             */
-            format: ModuleFormat;
+            format: string | ModuleFormat;


Ditto.

Suggested change

format: string | ModuleFormat;

format: string;

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

types/node/module.d.ts

            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).


Will apply the suggestion above here too if it's acceptable.

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é <[email protected]>

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

typescript-bot · 2025-02-14T11:13:15Z

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

typescript-bot · 2025-02-15T12:51:04Z

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

Renegade334 · 2025-02-15T18:44:48Z

@JakobJingleheimer How about 6bb9ba5? (The test suite is never executed, so it doesn't necessarily need to be comprehensive, it just needs to give the compiler an adequate example to test assignability.)

JakobJingleheimer · 2025-02-16T00:08:08Z

@JakobJingleheimer How about 6bb9ba5? (The test suite is never executed, so it doesn't necessarily need to be comprehensive, it just needs to give the compiler an adequate example to test assignability.)

OH! Sure, thanks!

P.S. Not needing to be robust is rather unintuitive though 😅

DangerBotOSS · 2025-02-16T00:26:55Z

Formatting

The following files are not formatted:

types/node/test/module.ts

Consider running pnpm dprint fmt on these files to make review easier.

Generated by 🚫 dangerJS against 9adb1ad

typescript-bot · 2025-02-16T00:27:33Z

@Renegade334, @Semigradsky 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?

JakobJingleheimer · 2025-02-23T13:30:24Z

@Renegade334 nudge 👀

typescript-bot · 2025-03-03T17:38:45Z

@JakobJingleheimer: Everything looks good here. I am ready to merge this PR (at 9adb1ad) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@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: you can do this too.)

JakobJingleheimer · 2025-03-03T17:40:02Z

Ready to merge

A `LoadHook` accepts a `LoadHookContext` object having a `format` of `string | null | undefined`. `LookHook` implementors often want to pass this value through ([example from docs](https://nodejs.org/api/module.html\#asynchronous-version), which suggests to me that a) it can be falsy, and b) it should be a looser type as established for `ResolveFnOutput` in DefinitelyTyped#71493). A consequence of this is that `ModuleFormat` is no longer referenced anywhere; it can be removed.

A `LoadHook` accepts a `LoadHookContext` object having a `format` of `string | null | undefined`. `LookHook` implementors often want to pass this value through ([example from docs](https://nodejs.org/api/module.html\#asynchronous-version), which suggests to me that a) it can be falsy, and b) it should be a looser type as established for `ResolveFnOutput` in DefinitelyTyped#71493).

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é <[email protected]>

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

update LoadFnOutput test for when context.format is not returnable

6b31256

make test case more rigid

9adb1ad

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 Feb 16, 2025

gabritto approved these changes Mar 3, 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 Mar 3, 2025

typescript-bot moved this from Needs Maintainer Action to Waiting for Author to Merge in Pull Request Status Board Mar 3, 2025

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in Pull Request Status Board Mar 3, 2025

typescript-bot merged commit eda5d24 into DefinitelyTyped:master Mar 3, 2025
15 checks passed

typescript-bot removed this from Pull Request Status Board Mar 14, 2025

boneskull mentioned this pull request Apr 23, 2025

[@types/node] correct LoadFnOutput.format #72581

Merged

8 tasks

	* A hint to the load hook (it might be ignored); can be an intermediary value.
	* A hint to the load hook (it might be ignored); can be an arbitrary value to be changed at a later stage (eg `css` → `json`).

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

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

Uh oh!

Conversation

JakobJingleheimer commented Dec 23, 2024

Uh oh!

typescript-bot commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Dec 23, 2024

Uh oh!

typescript-bot commented Dec 27, 2024

Uh oh!

JakobJingleheimer commented Dec 27, 2024

Uh oh!

JakobJingleheimer commented Dec 31, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renegade334 commented Jan 3, 2025

Uh oh!

typescript-bot commented Jan 3, 2025

Uh oh!

JakobJingleheimer commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakobJingleheimer commented Jan 3, 2025

Uh oh!

typescript-bot commented Jan 23, 2025

Uh oh!

typescript-bot commented Jan 26, 2025

Uh oh!

JakobJingleheimer commented Jan 26, 2025

Uh oh!

typescript-bot commented Feb 14, 2025

Uh oh!

typescript-bot commented Feb 15, 2025

Uh oh!

Renegade334 commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakobJingleheimer commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DangerBotOSS commented Feb 16, 2025

Formatting

Uh oh!

typescript-bot commented Feb 16, 2025

Uh oh!

JakobJingleheimer commented Feb 23, 2025

Uh oh!

typescript-bot commented Mar 3, 2025

Uh oh!

JakobJingleheimer commented Mar 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

[@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

Renegade334 commented Feb 15, 2025 •

edited

Loading

JakobJingleheimer commented Feb 16, 2025 •

edited

Loading