Remove whitespace by rbuckton · Pull Request #25492 · microsoft/TypeScript

rbuckton · 2018-07-06T22:29:22Z

This PR adds a --removeWhitespace compiler switch that affects JavaScript output only. When enabled, any insignificant whitespace (along with any insignificant trailing semicolon characters) is elided from the output.

Whitespace is considered significant in the following cases:

Between a keyword or identifier and another keyword or identifier.
Between a keyword or identifier and a number.
Between + and +
Between + and ++
Between - and -
Between - and --
Line terminators following a single-line comment

Semicolons are considered insignificant in the following cases:

The last ; before then end of a block (}), except when part of an embedded statement (i.e. {while(true);})
A run of multiple trailing semicolons (;;), except when in the head of a for loop (i.e. for(;;))

weswigham · 2018-07-06T23:31:34Z

src/compiler/emitter.ts

        }

+        function writeSignificantLine() {
+            // writer.flush();


Should either comment on why there's commented out code here or remove it.

This should be removed.

weswigham · 2018-07-06T23:34:40Z

src/compiler/sourcemap.ts

+            && typeof x.file === "string"
+            && typeof x.mappings === "string"
+            && isArray(x.sources) && every(x.sources, isString)
+            && (x.sourceRoot === undefined || typeof x.sourceRoot === "string")


Shouldn't we handle if sourceRoot === null? Same for sourcesContent and names?

I don't think null is legal per the spec, but we can be more permissive.

I'd rather make a best effort to parse a variety of bad sourcemaps than stubbornly refuse some almost correct ones.

weswigham · 2018-07-06T23:35:55Z

src/compiler/sourcemap.ts

+        return x !== null
+            && typeof x === "object"
+            && x.version === 3
+            && typeof x.file === "string"


While version and file are technically required in the spec.... we don't use them, and we don't need them to be present to handle a map correctly. Do we really need to check for them?

Its a small enough test on a non-hot code-path, and I'd prefer to align with the spec here.

weswigham · 2018-07-06T23:37:42Z

src/jsTyping/types.ts

@@ -1,4 +1,3 @@
-/* @internal */


This removal intentional?

Yes. Otherwise we get test failures for our Public API tests after building tsserverlibrary.d.ts.

Are these types supposed to be public (on master the library build is borked and including all private types)? This comment implies not...

They actually were public prior to the change to use project references:

https://github.com/Microsoft/TypeScript/blob/release-2.7/lib/tsserverlibrary.d.ts#L4845

Huh. OK. I guess we should ask Ryan if he intended to make them private or not.

I think the issue is that they weren't "public" in typescriptServices.d.ts/typescript.d.ts, but they are "public" in tsserverlibrary.d.ts. Unfortunately, we can't make a file conditionally internal.

@RyanCavanaugh - What's the best course of action here?

So the jsTyping file now contributes to the services build? Huh. I imagine it should be public then, considering they were already public in the server library; unless we've got a good reason to keep it private (at which point it should be private in both).

weswigham · 2018-07-06T23:40:09Z

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

@@ -1,21 +1,29 @@
+/*! *****************************************************************************


The comments in this file are now gone... except for the copyright header, which is now here. Seems wrong?

weswigham · 2018-07-06T23:41:26Z

tests/baselines/reference/removeWhitespace.outDir.js

+var a=0,b,{c}=obj,[d]=obj;let e=0,f,{g}=obj,[h]=obj;
+//# sourceMappingURL=variables.js.map//// [for.js]
+for(;;){}
+//# sourceMappingURL=for.js.map//// [embeddedStatement.js]


Hm. There's no tailing newline in the file, which is the goal, but this baseline is ugly because there's now a //// header on the same line as the last line as the prior file.

We weren't previously emitting a trailing newline after the source map comment. You can see this in existing baselines like jsxFactoryIdentifier.js, jsxFactoryQualifiedName.js, etc. We could probably fix this, but I wanted to avoid unrelated baseline changes.

:( we should open an issue to track it.

I'll just go ahead and fix it.

rbuckton · 2018-07-10T00:29:50Z

@sandersn, @mhegazy - If you have some time can you take a look?

rbuckton · 2018-07-10T21:56:55Z

@sandersn Should be smaller now with the build script changes pulled out.

sandersn

Looks fine as far as I could understand it with one reading. Maybe tomorrow I can sit down with you to learn some higher-level concepts to do with emitting and source maps.

sandersn · 2018-07-10T22:12:46Z

src/compiler/utilities.ts

@@ -61,11 +61,10 @@ namespace ts {
        let str = "";


not worth fixing as part of this PR, but utilities is a weird place for creating EmitTextWriters. Seems pretty specialised.

sandersn · 2018-07-10T22:52:37Z

src/compiler/sourcemap.ts

+        let nameToNameIndexMap: Map<number> | undefined;
+
+        // Last recorded and encoded mappings
+        let generatedLine = 0;


It's a bit confusing seeing a lot of references to generatedLine, sourceIndex, etc even though most are to locals, not to these closed-over declarations. Maybe another prefix to contrast with 'pending-' would help?

sandersn · 2018-07-10T23:11:21Z

src/compiler/sourcemap.ts

+            }
+
+            enter();
+            Debug.assert(pendingGeneratedCharacter >= 0, "lastRecordedGeneratedCharacter was negative");


I don't understand how the assert text relates to the assertion.

rbuckton · 2018-07-11T00:33:02Z

@weswigham if you have time can you take another look?

mhegazy

@weswigham can you take another look.

rbuckton · 2018-07-12T01:38:07Z

@mhegazy should I wait on @weswigham or merge?

mhegazy · 2018-07-12T18:47:08Z

go for it.

rbuckton · 2018-07-12T19:30:54Z

@DanielRosenwasser indicated we might want to wait on this a bit longer. I'll hold off on merging until we can discuss.

weswigham · 2018-08-01T23:53:28Z

Just one comment about the remove whitespace output: By it's nature, it is very hard to read (I'm pretty sure I just glossed over the new output baselines). It would be best if we could verify that they still behave as expected post-de-whitespacing. It's not necessary, strictly speaking, but just like with helper changes, these are exactly the kind of emit-related changes that actually validating the runtime behavior of the generated code seems useful to me. Just another remark for the technical backlog~ ❤️

sandersn · 2022-05-24T22:22:38Z

This experiment is pretty old, so I'm going to close it to reduce the number of open PRs.

rbuckton added 3 commits July 3, 2018 12:21

Gulp updates and api baseline fix

c1b37b1

Extract semicolon-omitting writer from printer

95a2055

Add whitespace-removing text writer

7df8091

rbuckton requested review from mhegazy, sandersn and weswigham July 6, 2018 22:29

weswigham reviewed Jul 6, 2018

View reviewed changes

rbuckton added 7 commits July 6, 2018 17:13

Merge branch 'master' into removeWhitespace

c22cca4

PR Feedback

ba2e4dc

Fix argument to decodeMappings

9e9c6a0

Ensure new line between js and map files in tests

c01783c

Merge branch 'master' into removeWhitespace

0b58e02

Ensure default os-invariant value for newLine in project tests

0654e6c

gulpfile/jakefile updates

5f29303

rbuckton requested a review from DanielRosenwasser July 9, 2018 20:51

rbuckton mentioned this pull request Jul 10, 2018

Build script updates #25556

Merged

Merge branch 'master' into removeWhitespace

a2d9fca

sandersn reviewed Jul 10, 2018

View reviewed changes

PR Feedback

57a68e0

mhegazy approved these changes Jul 11, 2018

View reviewed changes

rbuckton added 2 commits August 1, 2018 15:35

Merge branch 'master' into removeWhitespace

73b826c

Update diagnostic message code

108b1b4

weswigham approved these changes Aug 1, 2018

View reviewed changes

rbuckton added 2 commits August 2, 2018 11:06

Merge branch 'master' into removeWhitespace

5bdadb2

Merge branch 'master' into removeWhitespace

354e0c4

RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 17, 2018

rbuckton mentioned this pull request Oct 9, 2018

Simplify source map generation #27629

Merged

DanielRosenwasser modified the milestones: TypeScript 3.2, TypeScript 3.2.1 Nov 13, 2018

DanielRosenwasser modified the milestones: TypeScript 3.2.1, TypeScript 3.3.0 Nov 29, 2018

RyanCavanaugh assigned rbuckton Jan 25, 2019

RyanCavanaugh modified the milestones: TypeScript 3.3, TypeScript 3.4.0 Feb 1, 2019

RyanCavanaugh removed this from the TypeScript 3.4.0 milestone Apr 25, 2019

rbuckton mentioned this pull request Aug 15, 2019

Fix writing multi-line text without semicolons #32903

Merged

DDreamD approved these changes Nov 3, 2019

View reviewed changes

sandersn added the Experiment A fork with an experimental idea which might not make it into master label Feb 1, 2020

typescript-bot added the Author: Team label Feb 1, 2020

sandersn closed this May 24, 2022

microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025

		@@ -1,21 +1,29 @@
		/! ****************************************************************************

Conversation

rbuckton commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jul 10, 2018

Uh oh!

rbuckton commented Jul 10, 2018

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jul 11, 2018

Uh oh!

mhegazy left a comment

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jul 12, 2018

Uh oh!

mhegazy commented Jul 12, 2018

Uh oh!

rbuckton commented Jul 12, 2018

Uh oh!

weswigham commented Aug 1, 2018

Uh oh!

sandersn commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

weswigham Jul 6, 2018 •

edited

Loading

rbuckton Jul 7, 2018 •

edited

Loading