Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ tools/*/*.i.tmp
/tools/clang-format/node_modules
/tools/eslint/node_modules
/tools/lint-md/node_modules
/tools/typescript/node_modules

# === Rules for test artifacts ===
/*.tap
Expand Down
27 changes: 26 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -1468,6 +1468,29 @@ lint-js-ci: tools/eslint/node_modules/eslint/bin/eslint.js
jslint-ci: lint-js-ci
$(warning Please use lint-js-ci instead of jslint-ci)

# TypeScript type checking
tools/typescript/node_modules/typescript/bin/tsc: tools/typescript/package.json
-cd tools/typescript && $(call available-node,$(run-npm-ci))

.PHONY: typecheck-build
typecheck-build: ## Install TypeScript type checker dependencies
$(info Installing TypeScript for type checking...)
cd tools/typescript && $(call available-node,$(run-npm-ci))

.PHONY: typecheck
typecheck: ## Type-check TypeScript files in lib/
@if [ -f "tools/typescript/node_modules/typescript/bin/tsc" ]; then \
$(MAKE) run-typecheck ; \
else \
echo 'TypeScript type checking is not available'; \
echo "Run 'make typecheck-build'"; \
fi

.PHONY: run-typecheck
run-typecheck:
$(info Running TypeScript type checker...)
@$(call available-node,tools/typescript/node_modules/typescript/bin/tsc)

LINT_CPP_ADDON_DOC_FILES_GLOB = test/addons/??_*/*.cc test/addons/??_*/*.h
LINT_CPP_ADDON_DOC_FILES = $(wildcard $(LINT_CPP_ADDON_DOC_FILES_GLOB))
LINT_CPP_EXCLUDE ?=
Expand Down Expand Up @@ -1636,11 +1659,12 @@ lint: ## Run JS, C++, MD and doc linters.
$(MAKE) lint-addon-docs || EXIT_STATUS=$$? ; \
$(MAKE) lint-md || EXIT_STATUS=$$? ; \
$(MAKE) lint-yaml || EXIT_STATUS=$$? ; \
$(MAKE) typecheck || EXIT_STATUS=$$? ; \
exit $$EXIT_STATUS
CONFLICT_RE=^>>>>>>> [[:xdigit:]]+|^<<<<<<< [[:alpha:]]+

# Related CI job: node-test-linter
lint-ci: lint-js-ci lint-cpp lint-py lint-md lint-addon-docs lint-yaml-build lint-yaml
lint-ci: lint-js-ci lint-cpp lint-py lint-md lint-addon-docs lint-yaml-build lint-yaml typecheck
@if ! ( grep -IEqrs "$(CONFLICT_RE)" --exclude="error-message.js" --exclude="merge-conflict.json" benchmark deps doc lib src test tools ) \
&& ! ( $(FIND) . -maxdepth 1 -type f | xargs grep -IEqs "$(CONFLICT_RE)" ); then \
exit 0 ; \
Expand All @@ -1661,6 +1685,7 @@ lint-clean: ## Remove linting artifacts.
$(RM) .eslintcache
$(RM) -r tools/eslint/node_modules
$(RM) -r tools/lint-md/node_modules
$(RM) -r tools/typescript/node_modules
$(RM) tools/pip/site_packages

HAS_DOCKER ?= $(shell command -v docker > /dev/null 2>&1; [ $$? -eq 0 ] && echo 1 || echo 0)
Expand Down
2 changes: 1 addition & 1 deletion deps/amaro/lib/wasm.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ interface JsxConfig {



type Mode = "strip-only" | "transform";
export type Mode = "strip-only" | "transform";



Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
'use strict';

type Options = import('../../../deps/amaro/lib/wasm').Options;
type TransformOutput = import('../../../deps/amaro/lib/wasm').TransformOutput;
type Mode = 'strip-only' | 'transform';

const {
ObjectPrototypeHasOwnProperty,
} = primordials;
Expand All @@ -9,11 +13,13 @@ const {
validateObject,
validateString,
} = require('internal/validators');
const { assertTypeScript,
emitExperimentalWarning,
getLazy,
isUnderNodeModules,
kEmptyObject } = require('internal/util');
const {
assertTypeScript,
emitExperimentalWarning,
getLazy,
isUnderNodeModules,
kEmptyObject
} = require('internal/util');
Copy link
Member

Choose a reason for hiding this comment

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

Be aware that in a TS file, these kinds of imports will be entirely untyped because require is just a function that returns any and is not special-cased in TS files where you're supposed to use import util = require("internal/util").

Copy link
Member

Choose a reason for hiding this comment

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

Could the types be detected via something like } = /** @type {import('internal/util')} */(require('internal/util'))? Perhaps using more explicit paths in the @type.

Copy link
Member

Choose a reason for hiding this comment

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

Technically yes, though it's casting any to something so would be "unsafe" (but very easily identifiably correct).

Related-ish: microsoft/TypeScript#60032 (I can't find a better issue about const = require in TS, strangely)

Copy link
Member

Choose a reason for hiding this comment

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

The main reason not to do this is because it only gives you the value side of the import, so you can't use them in type positions.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry could you explain this in a bit more detail? Both what's wrong with my suggestion and what you would recommend instead, given the constraint that we can't use typescript syntax yet.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a fleshed out example of what happens in a TS file: Playground Link

This comes down to the fact that require the function is "just a function" when in a TS file; the binder does not bind it in such a way that we can go look up types.

We do this in JS files, though it is somewhat limited.

We've talked about doing this in TS files, but IIRC there are problems with doing so.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but I was suggesting the JSDoc syntax in a .js file; you say that tsc will “do this“ in JS files, so would my suggestion work?

And if not, that’s fine, but is there something else you could recommend? Even if we migrate Node internals to TypeScript, that’s probably a very long timeline effort and so we’ll have a lot of JS files for a long time. Getting type checking working on JS files in the meantime would be a win.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is converting some of the JS to TS, which is why I was bringing it up in this TS file. If you leave things in JS, then you don't need to think about this, of course.

Copy link
Member

Choose a reason for hiding this comment

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

I think we would like to add type checking to the Node codebase. Obviously that can be done by converting to TypeScript syntax, or adding JSDoc types to JavaScript; or a mix of both. We'll likely have a latter or a mix for a long time. Given that constraint, is there a way to add type checking to the files we have now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm sure you could use JSDoc (as clunky as it is).

const {
ERR_INVALID_TYPESCRIPT_SYNTAX,
ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING,
Expand All @@ -30,31 +36,26 @@ const {

/**
* The TypeScript parsing mode, either 'strip-only' or 'transform'.
* @type {function(): TypeScriptMode}
*/
const getTypeScriptParsingMode = getLazy(() =>
(getOptionValue('--experimental-transform-types') ?
(emitExperimentalWarning('Transform Types'), 'transform') : 'strip-only'),
const getTypeScriptParsingMode: () => Mode = getLazy(() =>
(getOptionValue('--experimental-transform-types') ?
(emitExperimentalWarning('Transform Types'), 'transform') : 'strip-only'),
);

/**
* Load the TypeScript parser.
* and returns an object with a `code` property.
* @returns {Function} The TypeScript parser function.
*/
const loadTypeScriptParser = getLazy(() => {
const loadTypeScriptParser: () => (source: string, options: Options) => TransformOutput = getLazy(() => {
assertTypeScript();
const amaro = require('internal/deps/amaro/dist/index');
const amaro: { transformSync: (source: string, options: Options) => TransformOutput } = require('internal/deps/amaro/dist/index');
return amaro.transformSync;
});

/**
*
* @param {string} source the source code
* @param {object} options the options to pass to the parser
* @returns {TransformOutput} an object with a `code` property.
* Parse TypeScript source code.
*/
function parseTypeScript(source, options) {
function parseTypeScript(source: string, options: Options): TransformOutput {
const parse = loadTypeScriptParser();
try {
return parse(source, options);
Expand Down Expand Up @@ -82,24 +83,18 @@ function parseTypeScript(source, options) {
}

/**
*
* @param {Error} error the error to decorate: ERR_INVALID_TYPESCRIPT_SYNTAX, ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX
* @param {object} amaroError the error object from amaro
* @returns {Error} the decorated error
* Decorate an error with a code snippet from the Amaro error.
*/
function decorateErrorWithSnippet(error, amaroError) {
function decorateErrorWithSnippet(error: Error, amaroError: any): Error {
const errorHints = `${amaroError.filename}:${amaroError.startLine}\n${amaroError.snippet}`;
error.stack = `${errorHints}\n${error.stack}`;
return error;
}

/**
* Performs type-stripping to TypeScript source code.
* @param {string} code TypeScript code to parse.
* @param {TransformOptions} options The configuration for type stripping.
* @returns {string} The stripped TypeScript code.
*/
function stripTypeScriptTypes(code, options = kEmptyObject) {
function stripTypeScriptTypes(code: string, options = kEmptyObject): string {
emitExperimentalWarning('stripTypeScriptTypes');
validateString(code, 'code');
validateObject(options, 'options');
Expand Down Expand Up @@ -127,22 +122,11 @@ function stripTypeScriptTypes(code, options = kEmptyObject) {
});
}

/**
* @typedef {'strip-only' | 'transform'} TypeScriptMode
* @typedef {object} TypeScriptOptions
* @property {TypeScriptMode} mode Mode.
* @property {boolean} sourceMap Whether to generate source maps.
* @property {string|undefined} filename Filename.
*/

/**
* Processes TypeScript code by stripping types or transforming.
* Handles source maps if needed.
* @param {string} code TypeScript code to process.
* @param {TypeScriptOptions} options The configuration object.
* @returns {string} The processed code.
*/
function processTypeScriptCode(code, options) {
function processTypeScriptCode(code: string, options: Options): string {
const { code: transformedCode, map } = parseTypeScript(code, options);

if (map) {
Expand All @@ -158,11 +142,8 @@ function processTypeScriptCode(code, options) {

/**
* Get the type enum used for compile cache.
* @param {TypeScriptMode} mode Mode of transpilation.
* @param {boolean} sourceMap Whether source maps are enabled.
* @returns {number}
*/
function getCachedCodeType(mode, sourceMap) {
function getCachedCodeType(mode: Mode, sourceMap: boolean): number {
if (mode === 'transform') {
if (sourceMap) { return kTransformedTypeScriptWithSourceMaps; }
return kTransformedTypeScript;
Expand All @@ -173,11 +154,8 @@ function getCachedCodeType(mode, sourceMap) {
/**
* Performs type-stripping to TypeScript source code internally.
* It is used by internal loaders.
* @param {string} source TypeScript code to parse.
* @param {string} filename The filename of the source code.
* @returns {TransformOutput} The stripped TypeScript code.
*/
function stripTypeScriptModuleTypes(source, filename) {
function stripTypeScriptModuleTypes(source: string, filename: string): string {
assert(typeof source === 'string');
if (isUnderNodeModules(filename)) {
throw new ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING(filename);
Expand All @@ -200,7 +178,7 @@ function stripTypeScriptModuleTypes(source, filename) {
return cached.transpiled;
}

const options = {
const options: Options = {
mode,
sourceMap,
filename,
Expand All @@ -219,12 +197,9 @@ function stripTypeScriptModuleTypes(source, filename) {
}

/**
*
* @param {string} code The compiled code.
* @param {string} sourceMap The source map.
* @returns {string} The code with the source map attached.
* Add a source map to compiled code.
*/
function addSourceMap(code, sourceMap) {
function addSourceMap(code: string, sourceMap: string): string {
// The base64 encoding should be https://datatracker.ietf.org/doc/html/rfc4648#section-4,
// not base64url https://datatracker.ietf.org/doc/html/rfc4648#section-5. See data url
// spec https://tools.ietf.org/html/rfc2397#section-2.
Expand Down
115 changes: 107 additions & 8 deletions src/node_builtins.cc
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,99 @@ BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const {
}

#ifdef NODE_BUILTIN_MODULES_PATH
static std::string OnDiskFileName(const char* id) {
std::string filename = NODE_BUILTIN_MODULES_PATH;
filename += "/";
static std::string OnDiskFileName(const char* id,
bool* is_typescript = nullptr) {
std::string base_path = NODE_BUILTIN_MODULES_PATH;
base_path += "/";

if (strncmp(id, "internal/deps", strlen("internal/deps")) == 0) {
id += strlen("internal/");
base_path += id + strlen("internal/");
} else {
filename += "lib/";
base_path += "lib/";
base_path += id;
}
filename += id;
filename += ".js";

return filename;
// Try .ts file first
std::string ts_filename = base_path + ".ts";
uv_fs_t req;
int r = uv_fs_stat(nullptr, &req, ts_filename.c_str(), nullptr);
uv_fs_req_cleanup(&req);

if (r == 0) {
if (is_typescript != nullptr) *is_typescript = true;
return ts_filename;
}

// Fall back to .js file
if (is_typescript != nullptr) *is_typescript = false;
return base_path + ".js";
}

// Strip TypeScript types by calling the JavaScript stripTypeScriptModuleTypes
// function
static MaybeLocal<String> StripTypeScriptTypes(Local<Context> context,
const std::string& source,
const std::string& filename) {
Isolate* isolate = context->GetIsolate();
EscapableHandleScope scope(isolate);

// Get the current environment
Environment* env = Environment::GetCurrent(context);
if (env == nullptr) {
// Runtime not initialized yet, cannot strip types
// Return the source as-is (this shouldn't happen in practice)
return String::NewFromUtf8(
isolate, source.c_str(), NewStringType::kNormal, source.length());
}

// Require the typescript module
Local<Value> typescript_module;
if (!env->builtin_module_require()
->Call(context,
Undefined(isolate),
1,
&FIXED_ONE_BYTE_STRING(isolate, "internal/modules/typescript")
.As<Value>())
.ToLocal(&typescript_module) ||
!typescript_module->IsObject()) {
// Cannot load typescript module, return source as-is
return String::NewFromUtf8(
isolate, source.c_str(), NewStringType::kNormal, source.length());
}

// Get the stripTypeScriptModuleTypes function
Local<Object> typescript_obj = typescript_module.As<Object>();
Local<Value> strip_fn;
if (!typescript_obj
->Get(context,
FIXED_ONE_BYTE_STRING(isolate, "stripTypeScriptModuleTypes"))
.ToLocal(&strip_fn) ||
!strip_fn->IsFunction()) {
// Function not found, return source as-is
return String::NewFromUtf8(
isolate, source.c_str(), NewStringType::kNormal, source.length());
}

// Call stripTypeScriptModuleTypes(source, filename)
Local<Value> args[2] = {
String::NewFromUtf8(
isolate, source.c_str(), NewStringType::kNormal, source.length())
.ToLocalChecked(),
String::NewFromUtf8(
isolate, filename.c_str(), NewStringType::kNormal, filename.length())
.ToLocalChecked()};

Local<Value> result;
if (!strip_fn.As<Function>()
->Call(context, Undefined(isolate), 2, args)
.ToLocal(&result) ||
!result->IsString()) {
// Stripping failed, return original source
return String::NewFromUtf8(
isolate, source.c_str(), NewStringType::kNormal, source.length());
}

return scope.Escape(result.As<String>());
}
#endif // NODE_BUILTIN_MODULES_PATH

Expand Down Expand Up @@ -283,6 +363,25 @@ MaybeLocal<Function> BuiltinLoader::LookupAndCompileInternal(
return {};
}

#ifdef NODE_BUILTIN_MODULES_PATH
// Check if this is a TypeScript file and strip types if needed
bool is_typescript = false;
std::string filename_for_check = OnDiskFileName(id, &is_typescript);

if (is_typescript) {
// Convert source to std::string
node::Utf8Value utf8_source(isolate, source);
std::string source_str(*utf8_source, utf8_source.length());

// Strip TypeScript types
if (!StripTypeScriptTypes(context, source_str, filename_for_check)
.ToLocal(&source)) {
// If stripping fails, use the original source
// (this will likely cause a syntax error, but better than crashing)
}
}
#endif // NODE_BUILTIN_MODULES_PATH

std::string filename_s = std::string("node:") + id;
Local<String> filename = OneByteString(isolate, filename_s);
ScriptOrigin origin(filename, 0, 0, true);
Expand Down
Loading
Loading