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

tools: enable type checking within the internal/modules directory #56500

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

@JakobJingleheimer JakobJingleheimer commented Jan 7, 2025

This enables us to find the current type issues within the modules subsystem. There are quite a few.

Not sure if we should:

  • merge this now and then fix (it could get annoying to see all the red, which is perhaps some dark UX to encourage people to fix them)
  • fix all the type issues as part of this
  • leave this open to the side whilst we merge in type fixes piecemeal, then merge this once they're all done

Perhaps we can eventually make this a CI check.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Jan 7, 2025
@JakobJingleheimer JakobJingleheimer added the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Jan 7, 2025
@legendecas
Copy link
Member

legendecas commented Jan 7, 2025

Type checking on JS files is not a nice experience -- to write types in JSDoc is sophisticated and making it enforced could lead to more sophisticated types and labor work in JSDoc to pass the check. I would prefer type stripping if enabling proper type check and a robust code base is what we eventually want.

/**
* @callback ImportModuleDynamicallyCallback
* @param {string} specifier
* @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
* @param {Record<string, string>} attributes
* @returns { Promise<void> }
*/
/**
* @callback InitializeImportMetaCallback
* @param {object} meta
* @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
*/
/**
* @typedef {{
* callbackReferrer: ModuleWrap|ContextifyScript|Function|vm.Module
* initializeImportMeta? : InitializeImportMetaCallback,
* importModuleDynamically? : ImportModuleDynamicallyCallback
* }} ModuleRegistry
*/
/**
* @type {WeakMap<symbol, ModuleRegistry>}
*/
const moduleRegistries = new SafeWeakMap();
/**
* @typedef {ContextifyScript|Function|ModuleWrap|ContextifiedObject} Referrer
* A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record
* as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule.
*
* In Node.js, a referrer is represented by a wrapper object of these records.
* A referrer object has a field |host_defined_option_symbol| initialized with
* a symbol.
*/

@JakobJingleheimer
Copy link
Member Author

I don't mind the jsdocs + type-checking (I use it in a couple projects). If we're open to type-stripping, that would be better! But a much more radical change I did not expect would get support.

@aduh95 aduh95 changed the title module: enable type checking tools: enable type checking on internal/modules Jan 7, 2025
@aduh95
Copy link
Contributor

aduh95 commented Jan 7, 2025

Let's use a commit message that makes it clear this is internal only, as is I thought you meant "in users' .ts files"

@JakobJingleheimer JakobJingleheimer force-pushed the test/enable-type-check-within-modules branch from 2dbe9b5 to 36ed7be Compare January 8, 2025 19:51
@JakobJingleheimer JakobJingleheimer changed the title tools: enable type checking on internal/modules tools: enable type checking within the internal/modules directory Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants