Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add tsconfig internal contributions #38042

Closed
wants to merge 8 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Apr 2, 2021

THIS PR IS NOT INTENDED TO MIGRATE TO USING THE TYPESCRIPT COMPILER.

THIS IS ONLY INTENDED TO HELP CORE DEVELOPMENT EXPERIENCE. In particular, this is to help people wishing to work on the project that may be less familiar with its internals.

So, right now code completions don't work so great for the average node core contributor compared to in other projects, a large factor to this is the lack of code completions due to the layout of source files in the project. This config file should help most IDEs with JavaScript language server support (through the TypeScript language server) do actual completions. This is not meant to make all the red squigglies you might see go away. I've however attached a before/after pic to somewhat show this, feel free to copy paste the file for some local testing.

From:

const { StringDecoder } = require('string_decoder');

Before:
Screen Shot 2021-04-02 at 07 43 52
After:
Screen Shot 2021-04-02 at 07 44 53

@bmeck bmeck requested a review from jasnell April 2, 2021 12:47
tsconfig.json Outdated Show resolved Hide resolved
@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

I gave this a fast try. On the one hand it works really great but VsCode complains about a few things now (e.g. in _http_outgoing.js):

Property 'Array' does not exist on type 'typeof primordials'.
Property 'ArrayIsArray' does not exist on type 'typeof primordials'.
...

By the way, I tried to detect the sorting algorithm used in the paths properties but failed...

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

@Flarna are the additional complaints from it actually detecting types rather than using any? We could add a file to declare the primordials but it would add a type declaration file to do so.

@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

I was able to fix this complains by adding a file typings/primeordials.d.ts with content like this:

declare module primordials {
  export const ArrayPrototypePush: typeof Array.prototype.push;
  export const FunctionPrototypeCall: typeof Function.prototype.call;
  export const ObjectDefineProperty: typeof Object.defineProperty;
  export const ObjectSetPrototypeOf: typeof Object.setPrototypeOf;
  export const StringPrototypeCharCodeAt: typeof String.prototype.charCodeAt;
  export const StringPrototypeSlice: typeof String.prototype.slice;
  export const StringPrototypeToLowerCase: typeof String.prototype.toLowerCase;
  export const Symbol: typeof globalThis.Symbol;
}

But then I get a log complains a lot like this (_http_incoming.js)

The 'this' context of type 'void' is not assignable to method's 'this' of type 'Function'.
Property '_readableState' does not exist on type 'IncomingMessage'.
Property 'on' does not exist on type 'IncomingMessage'.
...

@aduh95
Copy link
Contributor

aduh95 commented Apr 2, 2021

@Flarna I think you need to add .call to match the prototype method signatures:

declare module primordials {
  export const ArrayPrototypePush: typeof Array.prototype.push.call;
  export const FunctionPrototypeCall: typeof Function.prototype.call.call;
  export const ObjectDefineProperty: typeof Object.defineProperty;
  export const ObjectSetPrototypeOf: typeof Object.setPrototypeOf;
  export const StringPrototypeCharCodeAt: typeof String.prototype.charCodeAt.call;
  export const StringPrototypeSlice: typeof String.prototype.slice.call;
  export const StringPrototypeToLowerCase: typeof String.prototype.toLowerCase.call;
  export const Symbol: typeof globalThis.Symbol;
}

@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

@aduh95 That just increases the number of problems reported.

Before ArrayPrototypePush was fine but after adding .call VsCode reports The 'this' context of type 'void' is not assignable to method's 'this' of type 'Function'. at this line.

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

@aduh95 primordials need some specific stuff to actually be added and can't be done naively E.G. for Array.prototype.push => ArrayPush

type UncurryThis<T extends () => any> = (self: ThisParameterType<T>, ...args: Parameters<T>) => ReturnType<T>;
declare module primordials {
  export const ArrayPrototypePush: UncurryThis<Array.prototype.push>;
}

@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

@bmeck Even if I correct the primeordials pointing to prototype methods the problems like above remain. Seems typescript is not able to detect the inheritance for old style code using ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype);.

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

@Flarna yup, I don't expect this to magically make TS happy with node core, just to make it workable / able to actually check some things and properly goto definition for most simple things

@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

Yes that part works fine. But it's quite anoying if VsCode shows hundreds of problems.
It's a pitty that it's once again the magic which is missing...

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

@Flarna I think we could disable checking JS entirely (keep allowJs but set checkJs to false), the paths to modules would still work but all those red squiggles would go away (even if their causes did not)

@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

@bmeck Yes they go away, but unfortunatelly also the goto definition.
Which editor do you use? Is your editor behaving better then VsCode?

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

@Flarna I'm testing using VSCode @ 1.54.3

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you!

I've had to set this sort of file up manually when using VSCode for local Node.js development.

Even being familiar with where things are -> this change is still one less key-combo to press every time when working on Node.

@bmeck
Copy link
Member Author

bmeck commented Apr 2, 2021

I've added a preliminary typings file for primordials hopefully that is nicer and a less angry experience if you turn on checks

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems typings/primordials.d.ts doesn't include all the primordials (E.G.: ArrayPrototypeSymbolIterator and TypedArrayPrototypeGetLength are missing), and the utils are missing (primordials.uncurryThis, primordials.makeSafe, etc.).

What is the plan to keep the definition up-to-date? I suppose it would drift over time at each V8 updates..

typings/primordials.d.ts Outdated Show resolved Hide resolved
typings/primordials.d.ts Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Apr 5, 2021

It seems typings/primordials.d.ts doesn't include all the primordials (E.G.: ArrayPrototypeSymbolIterator and TypedArrayPrototypeGetLength are missing), and the utils are missing (primordials.uncurryThis, primordials.makeSafe, etc.).

These made TypeScript angry, uncurryThis cannot currently be represented in TS and makeSafe is a mutation that also can't be done in TS accurately.

What is the plan to keep the definition up-to-date? I suppose it would drift over time at each V8 updates..

We could make a build script that generates the definitions after building an executable. IDK if we want to do that automatically though, it shouldn't take long to run.

@bmeck
Copy link
Member Author

bmeck commented Apr 7, 2021

Are we ok leaving this file as manually maintained for now? I figure people can add anything missing in Primordials manually as we upgrade V8 and use those primordials in the codebase.

Other than that, I think I'd like to move forward with this after fixing the nits above.

@jasnell
Copy link
Member

jasnell commented Apr 7, 2021

Manually maintained is a good choice for now I think. We can figure out the rest as we go

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat rubber-stamp LGTM. I did look it over and I'm sure there are likely still issues to work through before it lands but definite +1 in principle.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some primordials that are missing in primordials.d.ts:

  • Methods defined by symbols (E.G.: ArrayPrototypeSymbolIterator)
  • Methods using applyBind instead of uncurryThis (E.G.: MathMaxApply, ArrayPrototypePushApply)
  • Getters/setters (E.G.: TypedArrayPrototypeGetLength)
  • Safe classes (E.G.: SafeMap, SafeArrayIterator)
  • Utility methods (E.G.:makeSafe, uncurryThis)

It'd be nice to have them defined, even as any, to avoid having VSCode reporting errors. But I agree this can be improved later, and IMHO it's OK to land as is.

typings/internalBinding.d.ts Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Apr 7, 2021

would it be possible to generate these somehow? make typings or something

@bmeck
Copy link
Member Author

bmeck commented Apr 7, 2021

@devsnek not to my knowledge, these are generated from runtime values and not statically

@bmeck
Copy link
Member Author

bmeck commented Apr 7, 2021

@devsnek i realize i probably misunderstood, do you mean a build step that could generate some of these that we could run manually? likely we could if we duplicate the logic of primordials.js and output types to disk instead of creating values.

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Apr 8, 2021

If anyone wants to prevent this landing due to something we can try to address it, but I'd prefer we try to land as is and iterate to better type support moving forward.

@devsnek
Copy link
Member

devsnek commented Apr 8, 2021

yeah this seems fine as long as the plan is to eventually do this automatically

@bmeck bmeck added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 8, 2021
bmeck added a commit that referenced this pull request Apr 8, 2021
PR-URL: #38042
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@bmeck
Copy link
Member Author

bmeck commented Apr 8, 2021

Landed in 656fb46

@bmeck bmeck closed this Apr 8, 2021
@targos targos added typings and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 11, 2021
@aduh95 aduh95 mentioned this pull request Dec 30, 2021
@bmeck bmeck deleted the make-code-completion-work branch February 3, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants