Remove whitespace generation by danez · Pull Request #5833 · babel/babel

danez · 2017-06-07T20:47:16Z

Q	A
Patch: Bug Fix?	n
Major: Breaking Change?	y
Minor: New Feature?	n
Deprecations?	n
Spec Compliancy?	n
Tests Added/Pass?	y
Fixed Tickets
License	MIT
Doc PR
Dependency Changes

This removes the whitespace generation and the generator would now simply print everything according to the default settings and not based on the AST tokens.
By removing this babel can use babylon without tokens, which brings a performance boost in babylon.

This change on it's own doesn't change the performance much. I tried printing react a 100 times with (14.8s) and without (15.0s) this change and it is neglectable, but at least not bad.

I removed all expected files and regenerated them.

loganfsmyth · 2017-06-07T20:51:33Z

packages/babel-generator/test/fixtures/comments/computed-property-comments-2/expected.js

   * Before bracket init
-   */
-  ["a"]: "1",
+   */["a"]: "1",


Seems like a bad case here.

loganfsmyth · 2017-06-07T20:52:43Z

packages/babel-generator/test/fixtures/comments/simple-a-lot-of-line-comment/expected.js

-function test() {}
-
-// Copyright (C) 2012 Yusuke Suzuki <utatane.tea@gmail.com>
+function test() {} // Copyright (C) 2012 Yusuke Suzuki <utatane.tea@gmail.com>


Hmm, also an unfortunate case.

loganfsmyth · 2017-06-07T20:56:06Z

Love this PR, anything to simplify codegen is a plus in my book.

I'd vote for including newlines after all block comments to make some of these examples nicer, and maybe to include a newline after all declaration statements?

danez · 2017-06-08T08:16:08Z

Yes will look into this, what I also noticed is that

function a () {
};

gets reprinted as

function a() {
}

;

But this is because of the EmptyStatement after the function. I wasn't sure how to fix that because the generation always prints a newline after a Node. And I didn't want to create special hacks in the code for EmptyStatement.

codecov · 2017-06-08T09:31:33Z

Codecov Report

Merging #5833 into 7.0 will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              7.0    #5833      +/-   ##
==========================================
- Coverage   85.25%   85.19%   -0.07%     
==========================================
  Files         284      283       -1     
  Lines        9963     9921      -42     
  Branches     2781     2765      -16     
==========================================
- Hits         8494     8452      -42     
+ Misses        969      967       -2     
- Partials      500      502       +2

Impacted Files	Coverage Δ
packages/babel-generator/src/generators/flow.js	`98.89% <100%> (-0.37%)`	⬇️
packages/babel-generator/src/index.js	`79.31% <100%> (-0.69%)`	⬇️
packages/babel-generator/src/node/whitespace.js	`96.05% <100%> (+3.86%)`	⬆️
packages/babel-generator/src/printer.js	`97.9% <100%> (-0.5%)`	⬇️
packages/babel-generator/src/generators/classes.js	`98.27% <0%> (-1.73%)`	⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js	`90.63% <0%> (ø)`	⬆️
packages/babel-traverse/src/path/context.js	`86.2% <0%> (ø)`	⬆️
packages/babel-traverse/src/visitors.js	`86.66% <0%> (+0.95%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5387d9f...80cef9c. Read the comment docs.

danez · 2017-06-08T09:32:49Z

Okay I change the generation now:

Add newline after last empty SwitchCase, to avoid

switch {
case 'a':
  break;
default:}

Add newlines around block comments if they are non-flow comments or contain newlines

I'm not sure how I could add a newline after every declaration.

I also made two commits. One with the code changes and one with the test fixes.

loganfsmyth · 2017-06-08T17:25:43Z

packages/babel-generator/test/fixtures/flow/call-properties/expected.js

-var a: { y: string, (): number, (x: string): string, };
-var a: { <T>(x: T): number };
-interface A { (): number };
+var a: { (): number


Weird asymmetry on these objects. I think if the closing bracket is on its own line, the opening one should be too.

loganfsmyth · 2017-06-08T17:26:34Z

packages/babel-generator/test/fixtures/flow/interfaces-module-and-script/expected.js

-interface A { foo: () => number };
-interface Dictionary { length: number, [index: string]: string, };
+interface A {}
+;


Are these actually parsed as EmptyStatement nodes, or is the semicolon being printed weirdly.

Not sure about this and how to approach. There is always an EmptyStatement generated after declarations with semicolon, and I'm not sure how we could ignore it if possible.

Looking at the AST from Flow itself, the semicolons are not in the grammar for interface declarations, which means the current behavior of parsing them as EmptyStatement nodes is correct. We should probably just remove the semicolons from the actual.js files, but I understand if you don't want to for now.

loganfsmyth · 2017-06-08T17:30:48Z

packages/babel-generator/src/printer.js

+      comment.type === "CommentBlock" &&
+      (
+        (!comment.value.startsWith(":") &&
+        !comment.value.startsWith("flow-include")) ||


Do we have any tests for this? Does Flow actually require these to be on the same line, or is this just for making it nicer to read? I don't love having a special case like this.

I just tested and they don't need to be on the same line. Will remove

Should I leave the case that only block comments with newlines inside them get newlines around them? Or add newlines around to all block comments?

So that inline block comments stay inline? like function a(a/*, b */) {}

I removed it now, all BlockComments have newline around them now.

hzoo · 2017-06-09T03:24:21Z

Are we removing end of file newlines now?

danez · 2017-06-09T08:33:13Z

Not sure about the actual output, in the fixture runner they are always trimmed. I will check

danez · 2017-06-17T16:50:07Z

@hzoo It seems to me we are always removing whitespaces at the end already here: https://github.com/babel/babel/blob/7.0/packages/babel-generator/src/buffer.js#L45

Although we might remove the trim-right here also, as it is probably irrelevant now if there is a whitespace at then end or not.

peey · 2017-06-18T09:08:47Z

Could we do this for code generation - don't worry too much about styling while generating code, but then pipe it through a linter's autofix for style consistency in the generated code?

That should free us from worrying about special cases like switch-case and empty statement.

Pros:

Standard output for the same input no matter the style

Cons:

Dependence on external tools for support of features that the generated code would use (shouldn't be a problem in most cases)

hzoo · 2017-06-19T04:34:24Z

then pipe it through a linter's autofix for style consistency in the generated code

The purpose was to make babel-generator faster so having to use another tool would go against this idea especially since it's easier to just encode the logic in the printer itself rather than in an autofixer. And if we were going to do that I would just use prettier as the printer anyway (which is much slower than hardcoding something simple)?

I think we just want it to look presentable but focus on speed. Again it's generated output not the source code (why we removed the options for quotes).

danez · 2017-06-20T14:27:25Z

Let me know if I should change more in this PR.

hzoo

@danez I'm totally good with this, I would just be more generous with adding newlines/whitespace (less checks to see if it's necessary or not) and then maybe figure out the ; thing so tests look more "normal" later

jridgewell · 2017-06-26T21:58:51Z

packages/babel-generator/src/node/whitespace.js

+  SwitchCase(node: Object, parent: Object): WhitespaceObject {
    return {
      before: node.consequent.length || parent.cases[0] === node,
+      after: !node.consequent.length && parent.cases[parent.cases.length - 1] === node,


This doesn't make sense.

Why not? It only covers the case of empty last case

switch() { default: }

Oh, i read this as two !node.consequent.length && last(node.consequent). Never mind.

jridgewell · 2017-06-26T21:59:12Z

packages/babel-generator/src/node/whitespace.js

+  if (
+    parent.callProperties[0] === node &&
+    (!parent.properties || !parent.properties.length)
+    ) {


Negative indent.

Changes to printing: * Add newline after last empty SwitchCase * Add newlines around block comments if they are non-flow comments or contain newlines

existentialism

I'll work on fixing the semis in the test fixtures

hzoo · 2017-06-28T02:57:37Z

Alright then!

danez · 2017-06-29T03:44:11Z

Thanks for taking care.

sebmck · 2017-06-29T19:37:47Z

For what it's worth, you can still retain newlines when printing a statement list by comparing the loc.end.line and loc.start.line of two adjacent statements and inserting the correct amount of newlines.

danez added the pkg: generator label Jun 7, 2017

danez requested review from hzoo and loganfsmyth June 7, 2017 20:47

loganfsmyth reviewed Jun 7, 2017

View reviewed changes

loganfsmyth reviewed Jun 8, 2017

View reviewed changes

hzoo approved these changes Jun 26, 2017

View reviewed changes

jridgewell reviewed Jun 26, 2017

View reviewed changes

danez and others added 2 commits June 27, 2017 21:30

Remove whitespace generation and rely on default printing

b2bb117

Changes to printing: * Add newline after last empty SwitchCase * Add newlines around block comments if they are non-flow comments or contain newlines

Fix a few more fixtures

20f5b8d

existentialism approved these changes Jun 28, 2017

View reviewed changes

existentialism merged commit b3372a5 into babel:7.0 Jun 28, 2017

hzoo added performance PR: Internal 🏠 A type of pull request used for our changelog categories labels Jun 28, 2017

existentialism mentioned this pull request Jun 28, 2017

Fix some unneeded semis in test fixtures #5892

Merged

danez deleted the remove-tokens branch June 29, 2017 03:43

hzoo added area: perf and removed area: perf performance labels Sep 13, 2017

hzoo mentioned this pull request Sep 16, 2017

Babel 7 twbs/bootstrap#23966

Merged

2 tasks

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Uh oh!

Conversation

danez commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganfsmyth commented Jun 7, 2017

Uh oh!

danez commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danez commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

danez Jun 8, 2017 • 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!

hzoo commented Jun 9, 2017

Uh oh!

danez commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danez commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peey commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzoo commented Jun 19, 2017

Uh oh!

danez commented Jun 20, 2017

Uh oh!

hzoo 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!

Choose a reason for hiding this comment

Uh oh!

existentialism left a comment

Choose a reason for hiding this comment

Uh oh!

hzoo commented Jun 28, 2017

Uh oh!

danez commented Jun 29, 2017

Uh oh!

sebmck commented Jun 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

danez commented Jun 7, 2017 •

edited

Loading

danez commented Jun 8, 2017 •

edited

Loading

codecov bot commented Jun 8, 2017 •

edited

Loading

danez commented Jun 8, 2017 •

edited

Loading

danez Jun 8, 2017 •

edited

Loading

danez commented Jun 9, 2017 •

edited

Loading

danez commented Jun 17, 2017 •

edited

Loading

peey commented Jun 18, 2017 •

edited

Loading