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

Use regular imports instead of require where possible #59017

Merged
merged 3 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
13 changes: 13 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@esfx/canceltoken": "^1.0.0",
"@octokit/rest": "^20.1.1",
"@types/chai": "^4.3.16",
"@types/diff": "^5.2.1",
"@types/minimist": "^1.2.5",
"@types/mocha": "^10.0.6",
"@types/ms": "^0.7.34",
Expand Down
18 changes: 3 additions & 15 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sourceMapSupport from "source-map-support";
import * as fakes from "./_namespaces/fakes.js";
import * as FourSlashInterface from "./_namespaces/FourSlashInterface.js";
import * as Harness from "./_namespaces/Harness.js";
Expand Down Expand Up @@ -4659,22 +4660,9 @@ function runCode(code: string, state: TestState, fileName: string): void {
const generatedFile = ts.changeExtension(fileName, ".js");
const wrappedCode = `(function(ts, test, goTo, config, verify, edit, debug, format, cancellation, classification, completion, verifyOperationIsCancelled, ignoreInterpolations) {${code}\n//# sourceURL=${ts.getBaseFileName(generatedFile)}\n})`;

type SourceMapSupportModule = typeof import("source-map-support") & {
// TODO(rbuckton): This is missing from the DT definitions and needs to be added.
resetRetrieveHandlers(): void;
};

// Provide the content of the current test to 'source-map-support' so that it can give us the correct source positions
// for test failures.
let sourceMapSupportModule: SourceMapSupportModule | undefined;
try {
sourceMapSupportModule = require("source-map-support");
}
catch {
// do nothing
}

sourceMapSupportModule?.install({
sourceMapSupport.install({
retrieveFile: path => {
return path === generatedFile ? wrappedCode :
undefined!;
Expand All @@ -4700,7 +4688,7 @@ function runCode(code: string, state: TestState, fileName: string): void {
throw err;
}
finally {
sourceMapSupportModule?.resetRetrieveHandlers();
sourceMapSupport.resetRetrieveHandlers();
}
}

Expand Down
15 changes: 4 additions & 11 deletions src/harness/harnessIO.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import * as Diff from "diff";
import fs from "fs";
import pathModule from "path";
import * as compiler from "./_namespaces/compiler.js";
import * as documents from "./_namespaces/documents.js";
import * as fakes from "./_namespaces/fakes.js";
Expand Down Expand Up @@ -54,14 +57,6 @@ export const virtualFileSystemRoot = "/";

function createNodeIO(): IO {
const workspaceRoot = Utils.findUpRoot();
let fs: any, pathModule: any;
if (require) {
fs = require("fs");
pathModule = require("path");
}
else {
fs = pathModule = {};
}

function deleteFile(path: string) {
try {
Expand Down Expand Up @@ -1022,7 +1017,6 @@ export namespace Compiler {
}
else if (original.text !== doc.text) {
jsCode += `\r\n\r\n!!!! File ${Utils.removeTestPathPrefixes(doc.file)} differs from original emit in noCheck emit\r\n`;
const Diff = require("diff");
const expected = original.text;
const actual = doc.text;
const patch = Diff.createTwoFilesPatch("Expected", "Actual", expected, actual, "The full check baseline", "with noCheck set");
Expand Down Expand Up @@ -1518,8 +1512,7 @@ export namespace Baseline {
IO.writeFile(actualFileName, encodedActual);
}
const errorMessage = getBaselineFileChangedErrorMessage(relativeFileName);
if (!!require && opts && opts.PrintDiff) {
const Diff = require("diff");
if (opts && opts.PrintDiff) {
const patch = Diff.createTwoFilesPatch("Expected", "Actual", expected, actual, "The current baseline", "The new version");
throw new Error(`${errorMessage}${ts.ForegroundColorEscapeSequences.Grey}\n\n${patch}`);
}
Expand Down
2 changes: 1 addition & 1 deletion src/harness/harnessUtils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import vm from "vm";
import * as Harness from "./_namespaces/Harness.js";
import * as ts from "./_namespaces/ts.js";

Expand All @@ -6,7 +7,6 @@ export function encodeString(s: string): string {
}

export function evalFile(fileContents: string, fileName: string, nodeContext?: any) {
const vm = require("vm");
if (nodeContext) {
vm.runInNewContext(fileContents, nodeContext, fileName);
}
Expand Down
18 changes: 9 additions & 9 deletions src/testRunner/parallel/host.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
import { fork } from "child_process";
import { statSync } from "fs";
import Mocha from "mocha";
import ms from "ms";
import os from "os";
import path from "path";
import readline from "readline";
import tty from "tty";
import {
configOption,
globalTimeout,
Expand Down Expand Up @@ -26,20 +34,12 @@ import * as ts from "../_namespaces/ts.js";
import * as Utils from "../_namespaces/Utils.js";

export function start(importTests: () => Promise<unknown>) {
const Mocha = require("mocha") as typeof import("mocha");
const Base = Mocha.reporters.Base;
const color = Base.color;
const cursor = Base.cursor;
const ms = require("ms") as typeof import("ms");
const readline = require("readline") as typeof import("readline");
const os = require("os") as typeof import("os");
const tty = require("tty") as typeof import("tty");
const isatty = tty.isatty(1) && tty.isatty(2);
const path = require("path") as typeof import("path");
const { fork } = require("child_process") as typeof import("child_process");
const { statSync } = require("fs") as typeof import("fs");

// NOTE: paths for module and types for FailedTestReporter _do not_ line up due to our use of --outFile for run.js
// NOTE: paths for module and types for FailedTestReporter _do not_ line up when bundled
const FailedTestReporter = require(Utils.findUpFile("scripts/failed-tests.cjs")) as typeof import("../../../scripts/failed-tests.cjs");

const perfdataFileNameFragment = ".parallelperf";
Expand Down
4 changes: 1 addition & 3 deletions src/testRunner/parallel/worker.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Mocha from "mocha";
import {
createRunner,
globalTimeout,
Expand Down Expand Up @@ -39,9 +40,6 @@ export function start(importTests: () => Promise<unknown>) {
let exceptionsHooked = false;
hookUncaughtExceptions();

// Capitalization is aligned with the global `Mocha` namespace for typespace/namespace references.
const Mocha = require("mocha") as typeof import("mocha");

/**
* Mixin helper.
* @param base The base class constructor.
Expand Down
3 changes: 1 addition & 2 deletions src/testRunner/unittests/skipJSDocParsing.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import * as Diff from "diff";
import * as Harness from "../_namespaces/Harness.js";
import * as ts from "../_namespaces/ts.js";
import * as Utils from "../_namespaces/Utils.js";

describe("unittests:: skipJSDocParsing", () => {
const Diff = require("diff");

const kinds = [
ts.JSDocParsingMode.ParseAll,
ts.JSDocParsingMode.ParseForTypeErrors,
Expand Down
86 changes: 10 additions & 76 deletions src/tsserver/nodeServer.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
import childProcess from "child_process";
import fs from "fs";
import net from "net";
import os from "os";
import readline from "readline";
Comment on lines +1 to +5
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 3, 2024

Choose a reason for hiding this comment

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

Is there any risk now that these are unconditional imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this is the tsserver project which only runs in node.

import {
CharacterCodes,
combinePaths,
Expand All @@ -9,7 +14,6 @@ import {
getDirectoryPath,
getRootLength,
LanguageServiceMode,
MapLike,
noop,
noopFileWatcher,
normalizePath,
Expand Down Expand Up @@ -37,24 +41,6 @@ interface LogOptions {
logToFile?: boolean;
}

interface NodeChildProcess {
send(message: any, sendHandle?: any): void;
on(message: "message" | "exit", f: (m: any) => void): void;
kill(): void;
pid: number;
}

interface ReadLineOptions {
input: NodeJS.ReadableStream;
output?: NodeJS.WritableStream;
terminal?: boolean;
historySize?: number;
}

interface NodeSocket {
write(data: string, encoding: string): boolean;
}

function parseLoggingEnvironmentString(logEnvStr: string | undefined): LogOptions {
if (!logEnvStr) {
return {};
Expand Down Expand Up @@ -123,41 +109,6 @@ function parseServerMode(): LanguageServiceMode | string | undefined {
/** @internal */
export function initializeNodeSystem(): StartInput {
const sys = Debug.checkDefined(ts.sys) as ts.server.ServerHost;
const childProcess: {
execFileSync(file: string, args: string[], options: { stdio: "ignore"; env: MapLike<string>; }): string | Buffer;
} = require("child_process");

interface Stats {
isFile(): boolean;
isDirectory(): boolean;
isBlockDevice(): boolean;
isCharacterDevice(): boolean;
isSymbolicLink(): boolean;
isFIFO(): boolean;
isSocket(): boolean;
dev: number;
ino: number;
mode: number;
nlink: number;
uid: number;
gid: number;
rdev: number;
size: number;
blksize: number;
blocks: number;
atime: Date;
mtime: Date;
ctime: Date;
birthtime: Date;
}

const fs: {
openSync(path: string, options: string): number;
close(fd: number, callback: (err: NodeJS.ErrnoException) => void): void;
writeSync(fd: number, buffer: Buffer, offset: number, length: number, position?: number): number;
statSync(path: string): Stats;
stat(path: string, callback?: (err: NodeJS.ErrnoException, stats: Stats) => any): void;
} = require("fs");

class Logger implements Logger {
private seq = 0;
Expand Down Expand Up @@ -231,7 +182,7 @@ export function initializeNodeSystem(): StartInput {
if (this.fd >= 0) {
const buf = Buffer.from(s);
// eslint-disable-next-line no-restricted-syntax
fs.writeSync(this.fd, buf, 0, buf.length, /*position*/ null!); // TODO: GH#18217
fs.writeSync(this.fd, buf, 0, buf.length, /*position*/ null); // TODO: GH#18217
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
}
if (this.traceToConsole) {
console.warn(s);
Expand Down Expand Up @@ -326,7 +277,7 @@ export function initializeNodeSystem(): StartInput {

let cancellationToken: ts.server.ServerCancellationToken;
try {
const factory = require("./cancellationToken");
const factory = require("./cancellationToken.js");
Copy link

Choose a reason for hiding this comment

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

Hi, I came across this line running dependency-cruiser on the typescript codebase. After building, it's the only require/import that can't be resolved. I notice that this file, nodeServer, does not have a cancellationToken sibling file - is it possible that this is always throwing (seems like it could be, since the catch block silently catches the error and uses ts.server.nullCancellationToken)

Copy link
Member Author

Choose a reason for hiding this comment

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

This file should exist as a sibling. https://unpkg.com/browse/typescript@5.6.3/lib/cancellationToken.js

Are you running this tool on the built code, or the source?

Copy link
Member Author

Choose a reason for hiding this comment

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

(realistically, it's not clear to me why this is a separate file at all anyway)

Copy link

Choose a reason for hiding this comment

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

Oh I see. I'm running it on the source, so I guess it's missing some part of the build process that compiles src/tsserver/nodeServer.ts into lib/tsserver.js, and src/cancellationToken/cancellationToken.ts into lib/cancellationToken.js, is that right? (Something in Hereby?)

Would it not work as require("../cancellationToken/cancellationToken.js") (which I think is what would be correct for the source file structure)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that would not work. Our input source structure has nothing to do with our output source structure; it's bundled and placed in a different directory.

Copy link

Choose a reason for hiding this comment

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

Got it - thanks very much!

Copy link
Member Author

Choose a reason for hiding this comment

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

#60250 you've inspired me

cancellationToken = factory(sys.args);
}
catch (e) {
Expand Down Expand Up @@ -439,31 +390,14 @@ function parseEventPort(eventPortStr: string | undefined) {
return eventPort !== undefined && !isNaN(eventPort) ? eventPort : undefined;
}
function startNodeSession(options: StartSessionOptions, logger: ts.server.Logger, cancellationToken: ts.server.ServerCancellationToken) {
const childProcess: {
fork(modulePath: string, args: string[], options?: { execArgv: string[]; env?: MapLike<string>; }): NodeChildProcess;
} = require("child_process");

const os: {
homedir?(): string;
tmpdir(): string;
} = require("os");

const net: {
connect(options: { port: number; }, onConnect?: () => void): NodeSocket;
} = require("net");

const readline: {
createInterface(options: ReadLineOptions): NodeJS.EventEmitter;
} = require("readline");

const rl = readline.createInterface({
input: process.stdin,
output: process.stdout,
terminal: false,
});

class NodeTypingsInstallerAdapter extends ts.server.TypingsInstallerAdapter {
protected override installer!: NodeChildProcess;
protected override installer!: childProcess.ChildProcess;
// This number is essentially arbitrary. Processing more than one typings request
// at a time makes sense, but having too many in the pipe results in a hang
// (see https://github.com/nodejs/node/issues/7657).
Expand Down Expand Up @@ -533,7 +467,7 @@ function startNodeSession(options: StartSessionOptions, logger: ts.server.Logger

const typingsInstaller = combinePaths(getDirectoryPath(sys.getExecutingFilePath()), "typingsInstaller.js");
this.installer = childProcess.fork(typingsInstaller, args, { execArgv });
this.installer.on("message", m => this.handleMessage(m));
this.installer.on("message", m => this.handleMessage(m as any));

// We have to schedule this event to the next tick
// cause this fn will be called during
Expand All @@ -550,7 +484,7 @@ function startNodeSession(options: StartSessionOptions, logger: ts.server.Logger

class IOSession extends ts.server.Session {
private eventPort: number | undefined;
private eventSocket: NodeSocket | undefined;
private eventSocket: net.Socket | undefined;
private socketEventQueue: { body: any; eventName: string; }[] | undefined;
/** No longer needed if syntax target is es6 or above. Any access to "this" before initialized will be a runtime error. */
private constructed: boolean | undefined;
Expand Down
3 changes: 2 additions & 1 deletion src/tsserver/server.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os from "os";
import * as ts from "../typescript/typescript.js";
import { StartInput } from "./common.js";
import { initializeNodeSystem } from "./nodeServer.js";
Expand Down Expand Up @@ -53,4 +54,4 @@ function start({ args, logger, cancellationToken, serverMode, unknownServerMode,
}

ts.setStackTraceLimit();
start(initializeNodeSystem(), require("os").platform());
start(initializeNodeSystem(), os.platform());
6 changes: 2 additions & 4 deletions src/typingsInstaller/nodeTypingsInstaller.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { execFileSync } from "child_process";
import * as fs from "fs";
import * as path from "path";

Expand Down Expand Up @@ -79,10 +80,8 @@ interface ExecSyncOptions {
cwd: string;
encoding: "utf-8";
}
type ExecSync = (command: string, options: ExecSyncOptions) => string;

export class NodeTypingsInstaller extends ts.server.typingsInstaller.TypingsInstaller {
private readonly nodeExecSync: ExecSync;
private readonly npmPath: string;
readonly typesRegistry: Map<string, MapLike<string>>;

Expand All @@ -109,7 +108,6 @@ export class NodeTypingsInstaller extends ts.server.typingsInstaller.TypingsInst
this.log.writeLine(`NPM location: ${this.npmPath} (explicit '${ts.server.Arguments.NpmLocation}' ${npmLocation === undefined ? "not " : ""} provided)`);
this.log.writeLine(`validateDefaultNpmLocation: ${validateDefaultNpmLocation}`);
}
({ execSync: this.nodeExecSync } = require("child_process"));

this.ensurePackageDirectoryExists(globalTypingsCacheLocation);

Expand Down Expand Up @@ -174,7 +172,7 @@ export class NodeTypingsInstaller extends ts.server.typingsInstaller.TypingsInst
this.log.writeLine(`Exec: ${command}`);
}
try {
const stdout = this.nodeExecSync(command, { ...options, encoding: "utf-8" });
const stdout = execFileSync(command, { ...options, encoding: "utf-8" });
if (this.log.isEnabled()) {
this.log.writeLine(` Succeeded. stdout:${indent(sys.newLine, stdout)}`);
}
Expand Down