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.

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

Refactor minify-numeric-literals. #349

Merged
merged 1 commit into from
Aug 18, 2017
Merged

Refactor minify-numeric-literals. #349

merged 1 commit into from
Aug 18, 2017

Conversation

bardiharborow
Copy link
Contributor

@bardiharborow bardiharborow commented Dec 18, 2016

I went ahead and rewrote the test suite for minify-numeric-literals, and came across a number of issues:

  • 0xa should transform to 10, instead of 0xa
  • 0x64 should transform to 100 instead of 1e2 (opinionated, better gzip?)
  • 0o12 should transform to 10 instead of 1e1
  • 0o144 should transform to 100 instead of 1e2 (opinionated, better gzip?)
  • 0b1010 should transform to 10 instead of 1e1
  • 0b1100100 should transform to 100 instead of 1e2 (opinionated, better gzip?)
  • 0.123456 should transform to .123456 instead of 0.123456
  • 1.5e3 should transform to 1500 instead of 1.5e3
  • 1e-1 should transform to .1 instead of 1e-1
  • 1e-2 should transform to .01 instead of 1e-2
  • 1e-3 should transform to .001 instead of 1e-3 (opinionated, better gzip?)
  • 1.5e-3 should transform to .0015 instead of 1.5e-3

I investigated several different ways of writing logic to solve this, but the simplest way I can find is to compare the length of .toString() and .toExponential() and to choose the shortest. Additionally I've removed the .replaceWith in favor of editing path.node.extra.raw in-place. This prevents the NumericLiteral from being re-entered immediately, but I'm unsure if this is good practice or not.

This PR has now been rewritten a little and incorporates the optimisations from #467.

Closes #467 and fixes #459.

@kangax
Copy link
Member

kangax commented Dec 20, 2016

Don't the last 5 cases already work this way? I can see it in a REPL. As far as leading 0, isn't this something that's controlled by Babel printer?

@bardiharborow
Copy link
Contributor Author

bardiharborow commented Dec 21, 2016

@kangax you are correct, but it's not minify-numeric-literals that's doing that, it's constant-folding. If I run my refactored tests without the associated plugin fixes, I get this:

Summary of all failing tests
 FAIL  packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js
  ● numeric-literals › should shorten hexadecimal literals correctly

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      "[1, 10, 100, 1e3, 0xfffffff, -1, -10, -100, -1e3, -0xfffffff];"
    Received:
      "[1, 0xa, 1e2, 1e3, 0xfffffff, -1, -0xa, -1e2, -1e3, -0xfffffff];"
      
      at Object.<anonymous> (packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js:24:31)

  ● numeric-literals › should shorten octal literals correctly

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      "[1, 10, 100, 1e3, -1, -10, -100, -1e3];"
    Received:
      "[1, 1e1, 1e2, 1e3, -1, -1e1, -1e2, -1e3];"
      
      at Object.<anonymous> (packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js:31:31)

  ● numeric-literals › should shorten binary literals correctly

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      "[1, 10, 100, 1e3, -1, -10, -100, -1e3];"
    Received:
      "[1, 1e1, 1e2, 1e3, -1, -1e1, -1e2, -1e3];"
      
      at Object.<anonymous> (packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js:38:31)

  ● numeric-literals › should shorten floats correctly

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      "[.123456, 1.23456, 12.3456, -.123456, -1.23456, -12.3456];"
    Received:
      "[.123456, 1.23456, 12.3456, -0.123456, -1.23456, -12.3456];"
      
      at Object.<anonymous> (packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js:45:31)

  ● numeric-literals › should shorten existing exponential literals correctly

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      "[10, 1e2, 1500, -10, -1e2, -1500, .1, .01, .0015];"
    Received:
      "[1e1, 1e2, 1.5e3, -1e1, -1e2, -1.5e3, 1e-1, 1e-2, 1.5e-3];"
      
      at Object.<anonymous> (packages/babel-plugin-minify-numeric-literals/__tests__/numeric-literals-test.js:61:31)


Test Suites: 1 failed, 26 passed, 27 total
Tests:       5 failed, 5 skipped, 430 passed, 440 total
Snapshots:   10 passed, 10 total
Time:        19.927s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

As for the leading zero, I'm unfamiliar with how the Babel printer works, but it seems to just echo what ever we give it in path.node.extra.raw.

@kangax
Copy link
Member

kangax commented Dec 22, 2016

I can't reproduce this (except for 0. removal).

> require('babel-core').transform('f(1e-1)', {presets:['babili']}).code
'f(0.1);'
> require('babel-core').transform('f(0xa)', {presets:['babili']}).code
'f(10);'
> require('babel-core').transform('f(0x64)', {presets:['babili']}).code
'f(100);'

@bardiharborow
Copy link
Contributor Author

bardiharborow commented Dec 29, 2016

Ah. The minified: true setting in the preset activates babel-generator/src/generators/types.js#L129, which already implements some of the behaviour I'm adding in my PR. I'd still recommend merging this, as it fixes behavior like changing [1000, 10000] to [1e3, 1e4], but it looks like you may be able to tweak babel-generator and drop minify-numeric-literals completely.

@boopathi
Copy link
Member

Can you rebase your branch? Looks like the CI didn't run.

.toExponential()
.replace(/\+/g, "")
.replace(/e0/, "");
const normal = path.node.value.toString().replace(/^0\./, ".");
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why I have used parseFloat instead of this, remember there was a missed optimisation.

@vigneshshanmugam
Copy link
Member

@bardiharborow Thanks a lot :)

@bardiharborow
Copy link
Contributor Author

@vigneshshanmugam Thanks for your extra optimisations too. :)
@boopathi CI is passing now.

@vigneshshanmugam vigneshshanmugam merged commit 0b282a6 into babel:master Aug 18, 2017
@boopathi boopathi added the Tag: New Feature Pull Request adding a new feature label Aug 18, 2017
@bardiharborow bardiharborow deleted the refactor-numeric-literals branch August 19, 2017 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: New Feature Pull Request adding a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number literal minification is suboptimal
4 participants