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.

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

Remove whitespace generation #5833

Merged
merged 2 commits into from Jun 28, 2017

Conversation

Projects
None yet
7 participants
@danez
Member

danez commented Jun 7, 2017

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.

@danez danez added the pkg: generator label Jun 7, 2017

@danez danez requested review from loganfsmyth and hzoo Jun 7, 2017

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

This comment has been minimized.

@loganfsmyth

loganfsmyth Jun 7, 2017

Member

Hmm, also an unfortunate case.

@loganfsmyth

loganfsmyth Jun 7, 2017

Member

Hmm, also an unfortunate case.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Jun 7, 2017

Member

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?

Member

loganfsmyth commented Jun 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 8, 2017

Member

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.

Member

danez commented Jun 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jun 8, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

codecov bot commented Jun 8, 2017

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 8, 2017

Member

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.

Member

danez commented Jun 8, 2017

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.

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

This comment has been minimized.

@loganfsmyth

loganfsmyth Jun 8, 2017

Member

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

@loganfsmyth

loganfsmyth Jun 8, 2017

Member

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

This comment has been minimized.

@danez

danez Jun 8, 2017

Member

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.

@danez

danez Jun 8, 2017

Member

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.

This comment has been minimized.

@loganfsmyth

loganfsmyth Jun 8, 2017

Member

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

loganfsmyth Jun 8, 2017

Member

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.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 9, 2017

Member

Are we removing end of file newlines now?

Member

hzoo commented Jun 9, 2017

Are we removing end of file newlines now?

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 9, 2017

Member

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

Member

danez commented Jun 9, 2017

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

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 17, 2017

Member

@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.

Member

danez commented Jun 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 18, 2017

Contributor

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)
Contributor

peey commented Jun 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 19, 2017

Member

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).

Member

hzoo commented Jun 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 20, 2017

Member

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

Member

danez commented Jun 20, 2017

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

@hzoo

hzoo approved these changes Jun 26, 2017

@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

Daniel Tschinder and others added some commits Jun 8, 2017

Remove whitespace generation and rely on default printing
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

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.68% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 28, 2017

Member

Alright then!

Member

hzoo commented Jun 28, 2017

Alright then!

@danez danez deleted the danez:remove-tokens branch Jun 29, 2017

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Jun 29, 2017

Member

Thanks for taking care.

Member

danez commented Jun 29, 2017

Thanks for taking care.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jun 29, 2017

Member

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.

Member

kittens commented Jun 29, 2017

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.

@hzoo hzoo referenced this pull request Sep 16, 2017

Merged

Babel 7 #23966

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment