lib: fix internalBinding type definitions

[anonrig]

Work in progress.

Current type definitions does not work for me. Investigating the issue. Full conversation is available on Twitter: https://twitter.com/andhaveaniceday/status/1642583763862552576?s=61&t=MpgxJm-yQa5OwwXgT9yE6A

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules
• @nodejs/startup

[jakebailey]

Ah, I can sorta see why things aren't working, or working strangely. Because Node's internals are not really modules, I think TS is detecting them as scripts all loaded into the same scope. That's probably hidden by not actually having checking enabled. I don't know how much work it'd be, but adding a tsconfig and adding allowJs/checkJs/noEmit might be interesting to try.

In the meantime, another pattern that you can use instead and keep the let is to instead use interface merging to declare the type of this. What you'd do is restore the /** @type {InternalBinding} */, then for each previous declaration of declare function InternalBinding, instead convert it to an interface with a signature.

For example, config.d.ts:

declare interface InternalBinding {
  (binding: 'config'): {
    isDebugBuild: boolean;
    hasOpenSSL: boolean;
    fipsMode: boolean;
    hasIntl: boolean;
    hasTracing: boolean;
    hasNodeOptions: boolean;
    hasInspector: boolean;
    noBrowserGlobals: boolean;
    bits: number;
    hasDtrace: boolean;
  }
}

This is the same approach used by the lib.d.ts files to add more overloads depending on the different lib target set in tsconfig.

Here's a patch you can apply if you revert the _internalBinding commit: change.patch

[jakebailey]

Adding a tsconfig file even if it doesn't have checkJs enabled may be desirable just so you can explicitly ensure that the d.ts files are loaded. Right now, I think they might only show up if you have opened the files in the editor.

Then afterward, you may be able to set moduleDetection=force to disable the scope sharing.

EDIT: I missed that you had a tsconfig already. Oops.

[jakebailey]

It seems to be working?

Some of them don't work because there are things missing from internalBinding but the ones that do exist seem to work fine.

It may be better to keep your renaming change, though, and then use the simpler option of overloads instead. It doesn't seem like the name matters in this loader file?

[anonrig]

Then afterward, you may be able to set moduleDetection=force to disable the scope sharing.

Current changes + moduleDetection=force did not make any difference for me.

It seems to be working?

Did you get it working with this PR changes + the git diff you've shared? I couldn't get it to working...

[jakebailey]

With this PR, reverting the _internalBinding change, and applying my diff, yes.

Another thing that worked without scope sharing was to instead declare var internalBinding, then var internalBinding: InternalBinding in a new d.ts file added to tsconfig, but that's not strictly required I don't think, maybe in moduleDetection=force if anything.

[anonrig]

With this PR, reverting the _internalBinding change, and applying my diff, yes.

It didn't work for me (both VSCode and Webstorm), it still detects isMainThread from internalBinding('worker') as any instead of boolean

[jakebailey]

Very very strange. Most of these work, but worker does not. I'm not sure what's going on here.

You probably don't want to load DOM to get URL; you'll pull in a load of stuff that won't work. You may just want to re-declare it locally, or copy from URL in @types/node.

[anonrig]

You probably don't want to load DOM to get URL; you'll pull in a load of stuff that won't work. You may just want to re-declare it locally, or copy from URL in @types/node.

Agreed. Removed it for the time being.

[gengjiawen]

With this PR, reverting the _internalBinding change, and applying my diff, yes.

It didn't work for me (both VSCode and Webstorm), it still detects isMainThread from internalBinding('worker') as any instead of boolean

Within this PR, vscode works (including worker), Clion not.

Maybe it's a cache problem in vscode (The ts server is buggy if it runs very long, have to restart it). Try restart ts service on your vscode.

Also online vsode works: https://gitpod.io/#https://github.com/nodejs/node/pull/47373/

CleanShot.2023-04-03.at.11.36.39-converted.mp4

[anonrig]

Within this PR, vscode works (including worker), Clion not.

@gengjiawen My main goal was to fix the Clion types... I'll leave this as a draft until I find a fix for Clion too...

[anonrig]

I finally got it working on Clion too...

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50882/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50940/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50961/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51091/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51200/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51220/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51226/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51229/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51235/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51239/

[gengjiawen]

From what I test, latest code not working for vscode.

[anonrig]

From what I test, latest code not working for vscode.

Thanks for testing @gengjiawen. I'll take a look at it

[anonrig]

@gengjiawen can you try it again?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51291/

[gengjiawen]

@gengjiawen can you try it again?

still not working.

[targos]

I confirm:

[anonrig]

OK, I'm officially giving up on this. Feel free to continue, if anyone is interested.

[gengjiawen]

@anonrig make it work on vscode enough for most people, we can take fix clion maybe later, reopen it ?