diff --git a/CLAUDE.md b/CLAUDE.md index 011f189c80..7c96cb1e69 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,6 +7,7 @@ - Never include `Co-Authored-By` lines in commit messages. ## Code Style +- This is a long-lived, partly legacy codebase — you will see older patterns like `var` in existing files. Leave surrounding legacy style alone unless you're deliberately refactoring it, but **any new code you write must use `const`/`let`, never `var`** (see below). Match the file's other conventions where they don't conflict with these rules. - 4-space indentation, never tabs. - Always use semicolons. - Brace style: (`if (x) {`), single-line blocks allowed. diff --git a/src/extensions/default/JSLint/JSHint.js b/src/extensions/default/JSLint/JSHint.js index 83e569ce4b..ae62d95674 100644 --- a/src/extensions/default/JSLint/JSHint.js +++ b/src/extensions/default/JSLint/JSHint.js @@ -40,8 +40,29 @@ define(function (require, exports, module) { FileSystem = brackets.getModule("filesystem/FileSystem"), IndexingWorker = brackets.getModule("worker/IndexingWorker"), Metrics = brackets.getModule("utils/Metrics"), + LanguageManager = brackets.getModule("language/LanguageManager"), ESLint = require("./ESLint"); + // JSHint defers to the TypeScript language service (vtsls) when it is linting the file (see + // canInspect). Its module (languageTools/LSPClient) is loaded lazily and only on desktop - it's + // intentionally kept out of the browser build - so resolve a handle there to query its state. When + // a server starts/stops, LSPClient re-runs inspection itself, so JSHint doesn't track that here. In + // the browser there is no LSP, so _lspClient stays null and JSHint keeps linting as before. + let _lspClient = null; + if (Phoenix.isNativeApp) { + brackets.getModule(["languageTools/LSPClient"], function (LSPClient) { + _lspClient = LSPClient; + }); + } + + function _lspLintingActive(fullPath) { + if (!_lspClient) { + return false; + } + const language = LanguageManager.getLanguageForPath(fullPath); + return !!language && _lspClient.isLintingProviderActive(language.getId()); + } + if(Phoenix.isTestWindow) { IndexingWorker.on("JsHint_extension_Loaded", ()=>{ window._JsHintExtensionReadyToIntegTest = true; @@ -269,9 +290,12 @@ define(function (require, exports, module) { scanFileAsync: lintOneFile, canInspect: function (fullPath) { return !prefs.get(PREFS_JSHINT_DISABLED) && fullPath && !fullPath.endsWith(".min.js") - && (isJSHintConfigActive() || !ESLint.isESLintActive()); - // if there is no linter, then we use jsHint as the default linter as it works in browser and native apps. - // remove ESLint.isESLintActive() once we add typescript language service that supports browser. + && (isJSHintConfigActive() + || (!ESLint.isESLintActive() && !_lspLintingActive(fullPath))); + // JSHint is the default JS linter only when nothing richer is linting the file: it defers + // to ESLint and to the TypeScript language service (vtsls, desktop). It still runs when a + // .jshintrc opts in explicitly, and as a fallback when neither is active (e.g. the browser, + // where there is no LSP, or when the language server isn't running). } }); diff --git a/src/extensions/default/JavaScriptCodeHints/main.js b/src/extensions/default/JavaScriptCodeHints/main.js index e69df616ae..37990a9ab3 100644 --- a/src/extensions/default/JavaScriptCodeHints/main.js +++ b/src/extensions/default/JavaScriptCodeHints/main.js @@ -59,6 +59,44 @@ define(function (require, exports, module) { var _inlineScriptLanguages = ["html", "php"], phProvider = new JSParameterHintsProvider(); + // --- Defer to the TypeScript language server (vtsls) when it is active ------------------------ + // When vtsls serves a JS/TS file, Tern is redundant: its hints lose to the server's higher- + // priority ones (see the provider registrations at priority 0 below) and its background project + // indexing only duplicates work the server already does. So on desktop we resolve a handle to + // LSPClient (intentionally kept out of the browser build) and, for any language the server serves, + // skip creating a Tern session - ScopeManager then never indexes the project in parallel with the + // server. The server starts lazily and can come up *after* Tern already indexed the first file, so + // we also listen for its start event to drop that data and re-evaluate the active editor; on stop + // we let Tern take over again. In the browser _lspClient stays null and Tern behaves as before. + // (We piggy-back on isLintingProviderActive as the "server is up and serving this language" + // signal - the same one JSHint uses to defer its linting.) + let _lspClient = null; + let _reinstallActiveEditorListeners = null; // assigned at appReady, once the listeners exist + + function _lspServesLanguage(languageId) { + return !!(_lspClient && languageId && _lspClient.isLintingProviderActive(languageId)); + } + + if (Phoenix.isNativeApp) { + brackets.getModule(["languageTools/LSPClient"], function (LSPClient) { + _lspClient = LSPClient; + LSPClient.on(LSPClient.EVENT_LANGUAGE_SERVER_STARTED, function () { + // The server may have started after Tern already indexed the first file - drop that + // analysis to reclaim the memory, then re-gate the active editor so it defers now. + ScopeManager.handleProjectClose(); + if (_reinstallActiveEditorListeners) { + _reinstallActiveEditorListeners(); + } + }); + LSPClient.on(LSPClient.EVENT_LANGUAGE_SERVER_STOPPED, function () { + // Server gone - let Tern resume handling the active editor. + if (_reinstallActiveEditorListeners) { + _reinstallActiveEditorListeners(); + } + }); + }); + } + // Define the detectedExclusions which are files that have been detected to cause Tern to run out of control. PreferencesManager.definePreference("jscodehints.detectedExclusions", "array", [], { description: Strings.DESCRIPTION_DETECTED_EXCLUSIONS @@ -663,7 +701,18 @@ define(function (require, exports, module) { return; } - if (editor && HintUtils.isSupportedLanguage(LanguageManager.getLanguageForPath(editor.document.file.fullPath).getId())) { + const languageId = editor + ? LanguageManager.getLanguageForPath(editor.document.file.fullPath).getId() + : null; + + // When the TypeScript language server serves this language, defer to it entirely: don't + // create a Tern session, so ScopeManager never indexes the project alongside the server. + if (_lspServesLanguage(languageId)) { + session = null; + return; + } + + if (editor && HintUtils.isSupportedLanguage(languageId)) { initializeSession(editor, previousEditor); editor .on(HintUtils.eventName("change"), function (event, editor, changeList) { @@ -845,6 +894,14 @@ define(function (require, exports, module) { ScopeManager.handleProjectOpen(); }); + // Lets the LSP start/stop handlers re-evaluate the active editor (gate Tern off when the + // server comes up, back on when it goes away) - defined here, where the listeners exist. + _reinstallActiveEditorListeners = function () { + const activeEditor = EditorManager.getActiveEditor(); + uninstallEditorListeners(activeEditor); + installEditorListeners(activeEditor); + }; + // immediately install the current editor installEditorListeners(EditorManager.getActiveEditor()); diff --git a/src/extensions/default/TypeScriptSupport/main.js b/src/extensions/default/TypeScriptSupport/main.js index 3ab4165db3..af4ee71838 100644 --- a/src/extensions/default/TypeScriptSupport/main.js +++ b/src/extensions/default/TypeScriptSupport/main.js @@ -33,6 +33,7 @@ define(function (require, exports, module) { const AppInit = brackets.getModule("utils/AppInit"), ProjectManager = brackets.getModule("project/ProjectManager"), DocumentManager = brackets.getModule("document/DocumentManager"), + EditorManager = brackets.getModule("editor/EditorManager"), FileSystem = brackets.getModule("filesystem/FileSystem"), NodeConnector = brackets.getModule("NodeConnector"), CodeIntelligence = require("./CodeIntelligence"); @@ -203,8 +204,7 @@ define(function (require, exports, module) { * @return {boolean} */ function canRun() { - return typeof Phoenix !== "undefined" && Phoenix.isNativeApp && - NodeConnector.isNodeAvailable && NodeConnector.isNodeAvailable(); + return Phoenix.isNativeApp && NodeConnector.isNodeAvailable(); } /** @@ -252,23 +252,71 @@ define(function (require, exports, module) { } } - // Begin loading the LSP framework as soon as the (desktop-only) extension loads - the - // reliable moment for module loading - so it is ready by the time start() runs. + // Begin loading the LSP framework as soon as the (desktop-only) extension loads - the reliable + // moment for module loading - so it is ready by the time we first need it. This only loads the + // module; it does not spawn the server (that happens lazily, on the first served-language file). if (canRun()) { loadLSPClient(); } + /** + * True when the active editor holds a language this server handles (JS/TS/JSX/TSX). + * @return {boolean} + */ + function _isServedLanguageActive() { + const editor = EditorManager.getActiveEditor(); + return !!(editor && SUPPORTED_LANGUAGES.indexOf(editor.getLanguageForSelection().getId()) !== -1); + } + + let starting = false; + let pendingRepoint = false; // a project switch happened; repoint once a served file is active there + let initErrorReported = false; // start() is retried lazily, so report a failure to telemetry only once + + /** + * Lazily start the language server when a served-language file is active, and - only right after a + * project switch - repoint the running server at the new root. Mirrors VS Code's onLanguage model: + * a project with no JS/TS file opened never spawns vtsls; switching to a non-JS project leaves the + * idle server where it was; and plain file switches within a project never touch the + * workspace-folder / restart machinery (so they can't interfere with a crash auto-restart). + */ + function _ensureServerForActiveEditor() { + if (!canRun() || !_isServedLanguageActive()) { + return; + } + + // Not running yet: lazily start it (a fresh start already points at the current project root). + if (!registered) { + if (starting) { + return; // a start kicked off by a previous activeEditorChange is still in flight + } + starting = true; + pendingRepoint = false; + start().catch(function (err) { + if (!initErrorReported) { + initErrorReported = true; + window.logger && window.logger.reportError(err, "[TypeScriptSupport] LSP init failed"); + } + }).finally(function () { + starting = false; + }); + return; + } + + // Running: repoint at the current project, but only when a project switch armed it - never on + // ordinary file switches. + if (pendingRepoint) { + pendingRepoint = false; + loadLSPClient().then(function (LSPClient) { + LSPClient.changeWorkspaceRoot(SERVER_ID); + }); + } + } + AppInit.appReady(function () { if (!canRun()) { return; } _refreshCheckJs(); - start().catch(function (err) { - console.error("[TypeScriptSupport] init failed", err && (err.message || err)); - }).finally(function () { - // Signal for integration tests that the server start has been attempted/settled. - window._TypeScriptSupportReadyToIntegTest = true; - }); // Offer project-wide code intelligence (creates a default ts/jsconfig) when a JS/TS file is // opened in a project that has no config yet. Projects that already carry one are silent. @@ -283,17 +331,20 @@ define(function (require, exports, module) { } }); - // Re-point the server at the new workspace root when the project changes, and re-evaluate - // whether the new project type-checks its JS. This uses workspace/didChangeWorkspaceFolders - // (no process restart, so no tsserver cold start) and only falls back to a full restart for - // servers that don't support live workspace-folder changes. + // Lazily start / repoint the server from the active editor's language (VS Code's onLanguage + // model). Evaluate the editor already open at startup (session restore), then track switches. + EditorManager.on("activeEditorChange", _ensureServerForActiveEditor); + _ensureServerForActiveEditor(); + + // On project switch: re-evaluate checkJs and arm a one-shot repoint. The actual repoint + // (workspace/didChangeWorkspaceFolders, no restart) happens the next time a served-language + // file is active - here if one already is, otherwise on the activeEditorChange as the new + // project's file opens. Plain file switches within a project never set this, so they don't + // repoint. ProjectManager.on(ProjectManager.EVENT_PROJECT_OPEN, function () { _refreshCheckJs(); - if (registered) { - loadLSPClient().then(function (LSPClient) { - LSPClient.changeWorkspaceRoot(SERVER_ID); - }); - } + pendingRepoint = true; + _ensureServerForActiveEditor(); }); // Pick up a tsconfig/jsconfig being added, edited, or removed at the project root. diff --git a/src/extensions/default/TypeScriptSupport/unittests.js b/src/extensions/default/TypeScriptSupport/unittests.js index 097a97721e..f4a39d3806 100644 --- a/src/extensions/default/TypeScriptSupport/unittests.js +++ b/src/extensions/default/TypeScriptSupport/unittests.js @@ -18,7 +18,7 @@ * */ -/*global describe, it, expect, beforeAll, afterAll, awaitsFor, awaitsForDone */ +/*global describe, it, expect, beforeAll, afterAll, awaitsFor, awaitsForDone, path, jsPromise */ define(function (require, exports, module) { @@ -55,10 +55,10 @@ define(function (require, exports, module) { CodeInspection = testWindow.brackets.test.CodeInspection; QuickViewManager = testWindow.brackets.getModule("features/QuickViewManager"); CodeInspection.toggleEnabled(true); - // Wait until the extension has attempted to start the language server. - await awaitsFor(function () { - return testWindow._TypeScriptSupportReadyToIntegTest; - }, "TypeScript LSP server to start", 30000); + // createTestWindowAndRun already waited for the app (and so the extension's appReady, which + // wires the lazy-start hooks) to finish loading. The server itself starts lazily on the + // first served-language file - the warm-up below opens a .ts, which starts it; waiting for + // its diagnostics ("not assignable") is the real readiness signal. // Warm up tsserver. Its very first request pays a large one-time cost - spawning node, // launching vtsls, and loading the TypeScript library + project - which on a slow/loaded @@ -125,6 +125,117 @@ define(function (require, exports, module) { expect(panelText().includes(IMPLICIT_ANY_MESSAGE)).toBe(false); }, 75000); + it("defers JSHint to the language server in a plain JS project", async function () { + // The language server is the JS linter on desktop, so the legacy JSHint linter must defer + // to it: the file below would draw a "Missing semicolon. jshint (W033)" nag if JSHint ran, + // but it must not appear. (No .jshintrc, so JSHint isn't explicitly opted in.) + const FileSystem = testWindow.brackets.test.FileSystem; + const LSPClient = await new Promise(function (resolve) { + testWindow.brackets.getModule(["languageTools/LSPClient"], resolve); + }); + // A plain JS project (js-plain has no jsconfig/tsconfig). Add a file JSHint would flag. + const projectPath = await SpecRunnerUtils.getTempTestDirectory(testRootSpec + "js-plain"); + await jsPromise(SpecRunnerUtils.createTextFile( + path.join(projectPath, "missing-semicolon.js"), 'console.log("hello")\n', FileSystem)); + await SpecRunnerUtils.loadProjectInTestWindow(projectPath); + await awaitsForDone(SpecRunnerUtils.openProjectFiles(["missing-semicolon.js"]), + "open missing-semicolon.js"); + + // The deferral only holds while the server is the active JS linter - wait for that. + await awaitsFor(function () { + return LSPClient.isLintingProviderActive("javascript"); + }, "the language server to be the active JS linter", 30000); + // The file is valid JS, so the server reports nothing; let inspection settle clean. + await awaitsFor(function () { + return $("#status-inspection").hasClass("inspection-valid"); + }, "inspection to settle with no problems", 30000); + expect(panelText().toLowerCase().includes("jshint")).toBe(false); + }, 45000); + + // ----- incremental document sync ---------------------------------------------------------- + + // DocumentSync sends incremental range edits (not the whole file) when the server advertises + // incremental sync. These two tests use the server's OWN diagnostics as the sync oracle: a + // type error can only appear or clear exactly on cue if the server's copy of the document + // matches the editor after every edit. If incremental replay ever drifted, the error would + // land on the wrong text (or not at all), failing the assertion. incremental.ts is mutated in + // memory only and force-closed without saving, so the on-disk fixture stays clean. + + function inspectionClean() { + return $("#status-inspection").hasClass("inspection-valid"); + } + + it("keeps the server in sync across many incremental edits", async function () { + await _openInProject("ts/", "incremental.ts"); + const editor = EditorManager.getCurrentFullEditor(); + const doc = editor.document; + await awaitsFor(inspectionClean, "incremental.ts to start clean", 30000); + + // 1) Many single-character inserts - each a separate change record - appending a valid + // statement. Fed only these incremental edits, the server must still parse it as clean. + const insert = "\nlet d: number = 4;"; + for (let i = 0; i < insert.length; i++) { + const line = editor.lineCount() - 1; + doc.replaceRange(insert[i], { line: line, ch: editor.getLine(line).length }); + } + await awaitsFor(inspectionClean, "still clean after many single-char inserts", 30000); + + // 2) A whole-line replacement (non-empty range) that introduces a type error on line 0. + doc.replaceRange('let a: number = "oops";', + { line: 0, ch: 0 }, { line: 0, ch: editor.getLine(0).length }); + await awaitsFor(function () { + return panelText().includes("not assignable"); + }, "type error after a mid-document line replacement", 30000); + + // 3) Fix it with another replacement -> the error must clear. + doc.replaceRange("let a: number = 0;", + { line: 0, ch: 0 }, { line: 0, ch: editor.getLine(0).length }); + await awaitsFor(inspectionClean, "error clears after the fix", 30000); + + // 4) A multi-line deletion (non-empty range, empty text): drop line 1 entirely. Still valid. + doc.replaceRange("", { line: 1, ch: 0 }, { line: 2, ch: 0 }); + await awaitsFor(inspectionClean, "still clean after a deletion", 30000); + + // Discard the in-memory edits so the fixture is pristine for re-runs / other tests. + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE, { _forceClose: true }), + "close incremental.ts"); + }, 90000); + + it("keeps the server in sync for a batched multi-cursor edit", async function () { + await _openInProject("ts/", "incremental.ts"); + const editor = EditorManager.getCurrentFullEditor(); + await awaitsFor(inspectionClean, "incremental.ts to start clean", 30000); + + // Two cursors on the SAME line, edited in one operation - the order-sensitive case, since + // the first replacement shifts the columns of the second. CodeMirror applies and reports + // the batch so that replaying the records in array order reproduces the result; this + // confirms our 1:1 change-record -> LSP-range map honours that ordering. + // "let a: number = 0;" -> "let abc: number = \"hello\";" + editor.setSelections([ + { start: { line: 0, ch: 4 }, end: { line: 0, ch: 5 } }, // the identifier `a` + { start: { line: 0, ch: 16 }, end: { line: 0, ch: 17 } } // the literal `0` + ]); + editor._codeMirror.replaceSelections(["abc", '"hello"']); + // Editor side is correct by construction; the assertion that matters is the server agreeing + // - it can only report the error if it received the same text. + expect(editor.getLine(0)).toBe('let abc: number = "hello";'); + await awaitsFor(function () { + return panelText().includes("not assignable"); + }, "type error after the multi-cursor edit", 30000); + + // Revert both cursors in one operation -> the error must clear (sync holds the other way). + editor.setSelections([ + { start: { line: 0, ch: 4 }, end: { line: 0, ch: 7 } }, // `abc` + { start: { line: 0, ch: 18 }, end: { line: 0, ch: 25 } } // `"hello"` + ]); + editor._codeMirror.replaceSelections(["a", "0"]); + expect(editor.getLine(0)).toBe("let a: number = 0;"); + await awaitsFor(inspectionClean, "error clears after reverting the multi-cursor edit", 30000); + + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE, { _forceClose: true }), + "close incremental.ts"); + }, 90000); + // ----- hover quick-actions (Go to Definition / Find Usages) ------------------------------- // Query the hover popover at a position the same way QuickViewManager does internally. diff --git a/src/languageTools/DefaultProviders.js b/src/languageTools/DefaultProviders.js index ab0e4d9a66..063c043fd0 100644 --- a/src/languageTools/DefaultProviders.js +++ b/src/languageTools/DefaultProviders.js @@ -201,6 +201,47 @@ define(function (require, exports, module) { return parts.join(""); } + // Minimal copy glyph (stroked, inherits currentColor) for the per-code-block copy button. + const COPY_ICON = ''; + + function _copyText(text) { + if (navigator.clipboard && navigator.clipboard.writeText) { + return navigator.clipboard.writeText(text); + } + return new Promise(function (resolve, reject) { + try { + const ta = document.createElement("textarea"); + ta.value = text; + ta.style.cssText = "position:fixed;opacity:0;pointer-events:none;"; + document.body.appendChild(ta); + ta.select(); + document.execCommand("copy"); + document.body.removeChild(ta); + resolve(); + } catch (e) { + reject(e); + } + }); + } + + // Give every code block in the doc popup a hover-revealed copy button. + function _addCopyButtons($popup) { + $popup.find("pre").each(function () { + const $pre = $(this); + if ($pre.children(".lsp-copy-btn").length) { + return; + } + $("