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.

Sign up for GitHub

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

Move tsserverlibrary.js to typescript.js, make tsserverlibrary.js a shim #55273

Merged
merged 8 commits into from
Aug 14, 2023
Merged

Move tsserverlibrary.js to typescript.js, make tsserverlibrary.js a shim #55273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f344f9b
Move tsserverlibrary.js to typescript.js
jakebailey May 3, 2023
ae24827
Make tsserverlibrary a wrapper around typescript
jakebailey Aug 4, 2023
b40497c
Remove redundant unit test covered by compiler case
jakebailey Aug 4, 2023
7d0cd83
Merge branch 'main' into bye-bye-tsserverlibrary
jakebailey Aug 9, 2023
d3fc401
Update shim to be less fancy
jakebailey Aug 10, 2023
1f6d337
Merge branch 'main' into bye-bye-tsserverlibrary
jakebailey Aug 14, 2023
e9c8a61
Re-add lssl to watch-local, which will run once
jakebailey Aug 14, 2023
0027c61
Fix diff
jakebailey Aug 14, 2023
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
51 changes: 37 additions & 14 deletions Herebyfile.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,27 +404,50 @@ export const watchMin = task({



const { main: lssl, build: buildLssl, watch: watchLssl } = entrypointBuildTask({
// This is technically not enough to make tsserverlibrary loadable in the
// browser, but it's unlikely that anyone has actually been doing that.
const lsslJs = `
if (typeof module !== "undefined" && module.exports) {
module.exports = require("./typescript.js");
}
else {
throw new Error("tsserverlibrary requires CommonJS; use typescript.js instead");
}
`;

const lsslDts = `
import ts = require("./typescript.js");
export = ts;
`;

const lsslDtsInternal = `
import ts = require("./typescript.internal.js");
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 is a little unfortunate for anyone who is attempting to pull these internal types, e.g. byots/ts-expose-internals but the fix would be quite simple for them; just use typescript.internal.d.ts for both.

export = ts;
`;

/**
* @param {string} contents
*/
async function fileContentsWithCopyright(contents) {
return await copyright() + contents.trim().replace(/\r\n/g, "\n") + "\n";
}

const lssl = task({
name: "lssl",
description: "Builds language service server library",
buildDeps: [generateDiagnostics],
project: "src/tsserverlibrary",
srcEntrypoint: "./src/tsserverlibrary/tsserverlibrary.ts",
builtEntrypoint: "./built/local/tsserverlibrary/tsserverlibrary.js",
output: "./built/local/tsserverlibrary.js",
mainDeps: [generateLibs],
bundlerOptions: { exportIsTsObject: true },
dependencies: [services],
run: async () => {
await fs.promises.writeFile("./built/local/tsserverlibrary.js", await fileContentsWithCopyright(lsslJs));
}
});
export { lssl, watchLssl };

export const dtsLssl = task({
name: "dts-lssl",
description: "Bundles tsserverlibrary.d.ts",
dependencies: [buildLssl],
dependencies: [dtsServices],
run: async () => {
if (needsUpdate("./built/local/tsserverlibrary/tsconfig.tsbuildinfo", ["./built/local/tsserverlibrary.d.ts", "./built/local/tsserverlibrary.internal.d.ts"])) {
await runDtsBundler("./built/local/tsserverlibrary/tsserverlibrary.d.ts", "./built/local/tsserverlibrary.d.ts");
}
await fs.promises.writeFile("./built/local/tsserverlibrary.d.ts", await fileContentsWithCopyright(lsslDts));
await fs.promises.writeFile("./built/local/tsserverlibrary.internal.d.ts", await fileContentsWithCopyright(lsslDtsInternal));
}
});

Expand Down Expand Up @@ -563,7 +586,7 @@ export const watchLocal = task({
name: "watch-local",
description: "Watches the full compiler and services",
hiddenFromTaskList: true,
dependencies: [localize, watchTsc, watchTsserver, watchServices, watchLssl, watchOtherOutputs, dts, watchSrc],
dependencies: [localize, watchTsc, watchTsserver, watchServices, lssl, watchOtherOutputs, dts, watchSrc],
Copy link
Member Author

Choose a reason for hiding this comment

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

Last minute change; we still have to run lssl here at least once in watch mode, so we do it just like dts here.

});

const runtestsDeps = [tests, generateLibs].concat(cmdLineOptions.typecheck ? [dts] : []);
Expand Down
16 changes: 0 additions & 16 deletions src/testRunner/unittests/publicApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as compiler from "../_namespaces/compiler";
import * as documents from "../_namespaces/documents";
import * as fakes from "../_namespaces/fakes";
import * as Harness from "../_namespaces/Harness";
Expand All @@ -19,21 +18,6 @@ describe("unittests:: Public APIs", () => {
it("should be acknowledged when they change", () => {
Harness.Baseline.runBaseline(api, fileContent, { PrintDiff: true });
});

it("should compile", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

We actually already test this by nature of the new public API compiler tests I added, which disable skipLibCheck to test the actual public API. This change could be pulled out into another PR but it's needed for this PR because this test assumes that our d.ts files are self-contained, which is no longer true.

Alternatively, I can just make tsserverlibrary.d.ts a copy of typescript.d.ts and it'd be fine.

Copy link
Member

Choose a reason for hiding this comment

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

is is possible to create symlinks for tsserverlibrary.js and tsserverLibrary,d,ts to point to typescript.js and typescript.d.ts instead

Copy link
Member Author

@jakebailey jakebailey Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now tsserverlibrary just reexports typescript, it's just that it's not self contained so this one particular test breaks (but isn't needed anyway).

In terms of the package itself, we can't really use symlinks as those are not universally supported on Windows or by package managers themselves. There are ways that people store packages which can't contain symlinks at all (e.g., zips, used in yarn, for example), and it'd be a bit dicey to commit back to a release branch.

const fs = vfs.createFromFileSystem(Harness.IO, /*ignoreCase*/ false);
fs.linkSync(`${vfs.builtFolder}/${fileName}`, `${vfs.srcFolder}/${fileName}`);
const sys = new fakes.System(fs);
const options: ts.CompilerOptions = {
...ts.getDefaultCompilerOptions(),
strict: true,
exactOptionalPropertyTypes: true,
lib: ["lib.es2018.d.ts"],
};
const host = new fakes.CompilerHost(sys, options);
const result = compiler.compileFiles(host, [`${vfs.srcFolder}/${fileName}`], options);
assert(!result.diagnostics || !result.diagnostics.length, Harness.Compiler.minimalDiagnosticsToString(result.diagnostics, /*pretty*/ true));
});
}

describe("for the language service and compiler", () => {
Expand Down
1 change: 0 additions & 1 deletion src/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
{ "path": "./testRunner" },
{ "path": "./tsc" },
{ "path": "./tsserver" },
{ "path": "./tsserverlibrary" },
{ "path": "./typescript" },
{ "path": "./typingsInstaller" },
{ "path": "./typingsInstallerCore" },
Expand Down
8 changes: 0 additions & 8 deletions src/tsserverlibrary/_namespaces/ts.ts
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions src/tsserverlibrary/tsconfig.json
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions src/tsserverlibrary/tsserverlibrary.ts
View file
Open in desktop

This file was deleted.

0 src/tsserverlibrary/_namespaces/ts.server.ts → src/typescript/_namespaces/ts.server.ts
View file
Open in desktop
File renamed without changes.
4 changes: 3 additions & 1 deletion src/typescript/_namespaces/ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@
export * from "../../compiler/_namespaces/ts";
export * from "../../jsTyping/_namespaces/ts";
export * from "../../services/_namespaces/ts";
export * from "../../deprecatedCompat/_namespaces/ts";
export * from "../../server/_namespaces/ts";
import * as server from "./ts.server";
export { server };
2 changes: 1 addition & 1 deletion src/typescript/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
{ "path": "../compiler" },
{ "path": "../jsTyping" },
{ "path": "../services" },
{ "path": "../deprecatedCompat" }
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional to drop drepcated compat from typescript.js ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already in the server tsconfig / project, since it's needed for both tsserver and tsserverlibrary; to make this PR I actually just deleted all of the contents of typescript and moved tsserverlibrary here, but git unfortunately can't show that!

Copy link
Member

Choose a reason for hiding this comment

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

it should still be here since references are not transitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

They aren't? But, tsserverlibrary's been set up this way since I merged modules and has been fine; what breaks if this isn't done? If it's about the code itself, it's imported in the server namespace so gets pulled in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing built and doing npx tsc -b ./src/typescript, I see:

image

Then hereby services gives me a file with:

image

So, it seems like it all works?

Copy link
Member Author

Choose a reason for hiding this comment

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

Build as in typecheck or as in write out files? I guess dts emit may matter for this...

I'm surprised in that I think a lot of monorepo projects aren't just bulk including all transitive dependencies in their tsconfigs as that would get unweildy.

Copy link
Member

Choose a reason for hiding this comment

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

#46153 and #30608

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha! Thanks!

I'll go figure out which transitive things we're missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pulled this out into #55339, which I'd like to merge first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given this isn't a regression (all I did was move tsserverlibrary and rename it), this shouldn't block merging of this PR, right? So the above isn't exactly a prereq?

{ "path": "../server" }
],
"include": ["**/*"]
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/APILibCheck.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import tsInternal = require("typescript-internal");
>tsInternal : typeof tsInternal

import tsserverlibrary = require("tsserverlibrary");
>tsserverlibrary : typeof tsserverlibrary
>tsserverlibrary : typeof ts

import tsserverlibraryInternal = require("tsserverlibrary-internal");
>tsserverlibraryInternal : typeof tsserverlibraryInternal
>tsserverlibraryInternal : typeof tsInternal

Loading