Deduplicate protocol.ts content #57361

andrewbranch · 2024-02-09T22:03:48Z

Fixes #57232 and improves some quality of life, hopefully

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

typescript-bot · 2024-02-09T22:04:03Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary.

src/server/protocol.ts

andrewbranch · 2024-02-09T22:05:22Z

src/server/protocol.ts

-/**
- * A set of one or more available refactoring actions, grouped under a parent refactoring.
- */
-export interface ApplicableRefactorInfo {


This is an example of an interface that was copied exactly from services/types.ts. This can simply be imported instead of duplicated.

andrewbranch · 2024-02-09T22:07:50Z

src/server/protocol.ts

-     */
-    kind: OutliningSpanKind;
-}
+export type OutliningSpan = ChangePropertyTypes<ts.OutliningSpan, { textSpan: TextSpan; hintSpan: TextSpan; }>;


This is an example of an interface that was copied from services/types.ts with a couple changed property types. I added a utility for this purpose. I only used it in cases where the majority of properties were identical; when there were many changes, I let the near-duplicate stand. Note that this is a homomorphic mapped type so JSDoc from the source type’s properties is retained.

andrewbranch · 2024-02-09T22:09:55Z

src/server/protocol.ts

+import {
+    ClassificationType,
+    CompletionTriggerKind,
+    OrganizeImportsMode,
+    SemicolonPreference,
+} from "./_namespaces/ts";


These were identical copies of const enums previously duplicated in this file, which can simply be imported now. Since we have preserveConstEnums on, they have to be imported without import type and re-exported below to ensure the shape of this module doesn’t change. (I validated this manually by comparing Object.keys(ts.server.protocol) before and after this PR.)

andrewbranch · 2024-02-09T22:15:29Z

src/server/protocol.ts

-    ReactNative = "ReactNative",
-    React = "React",
+    None = "none",
+    Preserve = "preserve",
+    ReactNative = "react-native",
+    React = "react",
+    ReactJSX = "react-jsx",
+    ReactJSXDev = "react-jsxdev",


All of these compiler options enums were missing values, but this one also had a value ReactNative = "ReactNative" which has forever been broken, as far as I can tell. When inferred or external compiler options are sent over the wire, enum-typed properties are allowed to be, for example, ts.JsxEmit | protocol.JsxEmit; i.e. a number or a corresponding string value. However, the conversion method simply uses the same parser as command-line or tsconfig values, which means these string values need to match the ones in the maps defined in commandLineParser.ts, which are kebab-cased. Uppercase/lowercase letters don’t matter, but "ReactNative" would not have worked since it was missing the hyphen.

I changed all of these enum values to lowercase for consistency, but react-native was the only one that strictly matters. This is technically a type-level breaking change.

andrewbranch · 2024-02-09T22:19:35Z

src/server/protocol.ts

+    type AssertKeysComplete<Source extends { [K in keyof Target]: any; }, Target> = Source;
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    type CopiedTypesComplete = [
+        AssertKeysComplete<typeof ModuleResolutionKind, typeof ts.ModuleResolutionKind>,
+        AssertKeysComplete<typeof ModuleKind, typeof ts.ModuleKind>,
+        AssertKeysComplete<typeof ScriptTarget, typeof ts.ScriptTarget>,
+        AssertKeysComplete<typeof JsxEmit, typeof ts.JsxEmit>,
+        AssertKeysComplete<typeof IndentStyle, typeof ts.IndentStyle>,
+    ];


As mentioned before, what really matters is that the stringified enums represent every value for each option that the command line parser can recognize. Those strings aren’t encoded in types anywhere else that I know of, so rather than checking that directly, these checks just make sure each enum has all the keys the source enum has, protecting us from forgetting to update the protocol when compiler options get new values. The enum values here in the protocol could still be typo’d, as was the case for "ReactNative"

typescript-bot · 2024-02-09T22:21:44Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

src/server/protocol.ts

jakebailey · 2024-03-01T21:25:20Z

scripts/dtsBundler.mjs

                // We didn't find the symbol in scope at all. Just allow it and we'll fail at test time.
                return node;


Maybe not for this PR, but I do feel a bit like I should have originally just made this error.

jakebailey · 2024-03-01T21:27:22Z

scripts/dtsBundler.mjs

@@ -310,6 +318,10 @@ function verifyMatchingSymbols(decl) {
            }
            const symbolInScope = findInScope(symbolOfNode.name);
            if (!symbolInScope) {
+                if (symbolOfNode.declarations?.every(d => isLocalDeclaration(d) && d.getSourceFile() === decl.getSourceFile()) && !isSelfReference(node, symbolOfNode)) {
+                    // The symbol is a local that needs to be copied into the scope.
+                    scopeStack[scopeStack.length - 1].locals.set(symbolOfNode.name, { symbol: symbolOfNode, writeTarget: isInternal ? WriteTarget.Internal : WriteTarget.Both });


It seems moderately weird for this verification function to modify the locals, but not sure what's better.

I feel like I considered either renaming the function or returning the set of new locals to be appended before I paused this work.

@jakebailey any more thoughts on this? I agree this code isn’t great. Otherwise I think the current state is ready for review.

No, not really; I think it's fine for now. Thankfully I don't think the bundler has that high of a cognitive load nor changes that much 😄

tests/baselines/reference/api/typescript.d.ts

jakebailey

LGTM, though at some level I wonder if we should switch everything to export now such that future PRs which modify the top level API don't have to incur the diff change. But, another PR for that would be fine.

(I'm waffling on whether or not an export modifier is good or bad at this point; before we didn't have any so I was happy because it meant a smaller package.)

andrewbranch and others added 3 commits February 9, 2024 12:20

Deduplicate protocol.ts content

d6cc6c5

Fix dtsBundler

99985d8

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

Fix missing properties and exports

e9db509

typescript-bot assigned andrewbranch Feb 9, 2024

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 9, 2024

andrewbranch requested a review from jakebailey February 9, 2024 22:03

andrewbranch requested a review from sheetalkamat February 9, 2024 22:04

jakebailey reviewed Feb 9, 2024

View reviewed changes

src/server/protocol.ts Show resolved Hide resolved

andrewbranch commented Feb 9, 2024

View reviewed changes

Update API baseline

eb157f8

jakebailey reviewed Feb 9, 2024

View reviewed changes

src/server/protocol.ts Outdated Show resolved Hide resolved

andrewbranch added 3 commits March 1, 2024 12:55

Detect and preserve local helper types in dtsBundler

b653034

Merge branch 'main' into protocol-deduplication

9e70d47

Update API baseline

83eceac

jakebailey reviewed Mar 1, 2024

View reviewed changes

Update baselines with changed casing

56da82b

jakebailey reviewed Mar 1, 2024

View reviewed changes

tests/baselines/reference/api/typescript.d.ts Outdated Show resolved Hide resolved

Fix dtsBundler emit for namespace declarations

e531c0b

andrewbranch force-pushed the protocol-deduplication branch from fe72547 to e531c0b Compare March 1, 2024 23:03

sheetalkamat approved these changes Mar 4, 2024

View reviewed changes

jakebailey approved these changes Mar 4, 2024

View reviewed changes

andrewbranch merged commit 6d458e8 into microsoft:main Mar 4, 2024
19 checks passed

andrewbranch deleted the protocol-deduplication branch March 4, 2024 19:41

This was referenced May 6, 2024

Add preferTypeOnlyAutoImports to protocol.UserPreferences #58389

Closed

Re-export more referenced types from protocol.ts #58487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate protocol.ts content #57361

Deduplicate protocol.ts content #57361

andrewbranch commented Feb 9, 2024

typescript-bot commented Feb 9, 2024

andrewbranch Feb 9, 2024

andrewbranch Feb 9, 2024

andrewbranch Feb 9, 2024

andrewbranch Feb 9, 2024

andrewbranch Feb 9, 2024

typescript-bot commented Feb 9, 2024

jakebailey Mar 1, 2024

jakebailey Mar 1, 2024

andrewbranch Mar 1, 2024

andrewbranch Mar 4, 2024

jakebailey Mar 4, 2024

jakebailey left a comment

		// We didn't find the symbol in scope at all. Just allow it and we'll fail at test time.
		return node;

Deduplicate protocol.ts content #57361

Deduplicate protocol.ts content #57361

Conversation

andrewbranch commented Feb 9, 2024

typescript-bot commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment