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

BigInt (Stage 3) [WIP] #6015

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@wdhorton
Contributor

wdhorton commented Jul 26, 2017

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

I'm working on the transform for BigInt, and I wanted to put up a PR to get some feedback on the direction it's going.

This just works for adding two BigInts right now, obviously there are other operations, errors to be caught, edge cases, etc., etc. but I think if we can nail down the details of this case I should be able to move forward pretty quickly.

Main questions:
For @littledan:

For babel people (@hzoo or others):

  • When I want to have the polyfill in the transformed file, should I be using import or require? I went with import because there's an addImport method and there doesn't seem to be an analogous state.file.addRequire method, but that breaks my tests unless I use transform-es2015-modules-commonjs as an additional plugin.

  • Should I be importing/requiring the BigInt polyfill as a variable, or should I be putting it on the global as a side effect of the import/require? How would that work if people use the BigInt constructor themselves in the code they want to transform?

  • Where should the code for the polyfill live? It's going to be a thin wrapper around an existing BigNumber library. Right now "bigint-polyfill" is just a placeholder name. Should it be in babel-helpers? babel-polyfill? A separate NPM package (that seems less than ideal for development purposes).

  • Any additional feedback on whether or not I'm on the right track with this would be much appreciated.

@wdhorton wdhorton referenced this pull request Jul 26, 2017

Open

BigInt (Stage 3) #2

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jul 26, 2017

Member

Thanks for posting your current work, nice stuff! As a general thing you'll want to do some of the things in https://github.com/babel/babel/wiki/Adding-a-new-Proposal-to-Babel (generator to print it out, babel types to create a bigint)

Member

hzoo commented Jul 26, 2017

Thanks for posting your current work, nice stuff! As a general thing you'll want to do some of the things in https://github.com/babel/babel/wiki/Adding-a-new-Proposal-to-Babel (generator to print it out, babel types to create a bigint)

@wdhorton

This comment has been minimized.

Show comment
Hide comment
@wdhorton

wdhorton Aug 9, 2017

Contributor

@hzoo @littledan made changes based on your feedback. I've got the addition case passing for BigInts, and not breaking for normal numbers.

A few more questions:

  • I added BigInt to babel-polyfill because I realized that, to match the spec, the constructor should be available in the global scope, so people can write:
BigInt('1223456')

in addition to the literal form 1223456n. I'd like to make sure that's the right idea, and if so, should I change anything about how I integrated it into babel-polyfill

  • I was still struggling to find the right place in the codebase for the function to check binary expressions. I started out trying to add it to babel-helpers, since that seemed like the natural place for helper functions, but I ran into an issue because if you include a helper, the code from that helper is run through the transform. To illustrate, the helper code:
(function(left, right) {
  if (left instanceof BigInt && right instanceof BigInt) {
    return left.plus(right);
  } else {
    return left + right;
  }
})

was transformed to this:

function _checkBinaryExpression(left, right) { 
  if (left instanceof BigInt && right instanceof BigInt) { 
    return left.plus(right); 
  } else { 
    return _checkBinaryExpression(left, right); 
  } 
}

which caused infinite recursion on non-BigInt numbers. I might be missing something, but it seems like I can't include BinaryExpressions in the helper code if the whole thing the transform does is change BinaryExpressions into calls to the helper.

So for now I just stuck it in another package (babel-check-binary-expressions), but I'd like to discuss what the best way to handle this is.

Contributor

wdhorton commented Aug 9, 2017

@hzoo @littledan made changes based on your feedback. I've got the addition case passing for BigInts, and not breaking for normal numbers.

A few more questions:

  • I added BigInt to babel-polyfill because I realized that, to match the spec, the constructor should be available in the global scope, so people can write:
BigInt('1223456')

in addition to the literal form 1223456n. I'd like to make sure that's the right idea, and if so, should I change anything about how I integrated it into babel-polyfill

  • I was still struggling to find the right place in the codebase for the function to check binary expressions. I started out trying to add it to babel-helpers, since that seemed like the natural place for helper functions, but I ran into an issue because if you include a helper, the code from that helper is run through the transform. To illustrate, the helper code:
(function(left, right) {
  if (left instanceof BigInt && right instanceof BigInt) {
    return left.plus(right);
  } else {
    return left + right;
  }
})

was transformed to this:

function _checkBinaryExpression(left, right) { 
  if (left instanceof BigInt && right instanceof BigInt) { 
    return left.plus(right); 
  } else { 
    return _checkBinaryExpression(left, right); 
  } 
}

which caused infinite recursion on non-BigInt numbers. I might be missing something, but it seems like I can't include BinaryExpressions in the helper code if the whole thing the transform does is change BinaryExpressions into calls to the helper.

So for now I just stuck it in another package (babel-check-binary-expressions), but I'd like to discuss what the best way to handle this is.

@caiolima

That's a very good job!

Show outdated Hide outdated packages/babel-check-binary-expressions/src/index.js
@@ -0,0 +1 @@
assert.equal((1000n + 200n).toString(), (new BigInt('1200')).toString())

This comment has been minimized.

@caiolima

caiolima Aug 9, 2017

Add test cases when you have BigInt + other primitive.

@caiolima

caiolima Aug 9, 2017

Add test cases when you have BigInt + other primitive.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Aug 29, 2017

I'm wondering, how are you running this code? Do you have a BigInt library that you are using? Or are you just examining the generated output?

littledan commented Aug 29, 2017

I'm wondering, how are you running this code? Do you have a BigInt library that you are using? Or are you just examining the generated output?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 29, 2017

Member

screen shot 2017-08-29 at 11 19 08 am

maybe if you rebase you could test this with our new repl links, although not sure how it would add the polyfill until we figure out a way to import those

Member

hzoo commented Aug 29, 2017

screen shot 2017-08-29 at 11 19 08 am

maybe if you rebase you could test this with our new repl links, although not sure how it would add the polyfill until we figure out a way to import those

@wdhorton

This comment has been minimized.

Show comment
Hide comment
@wdhorton

wdhorton Aug 30, 2017

Contributor

@littledan i just shuffled stuff around a bit, but I made a BigInt polyfill that wraps around the big-integer package. That part of the PR is here. In the test environment, Babel loads the polyfills, so that's what's on global.BigInt. The tests that are named exec.js are actually executing the code, so I'm using that to test that it runs, in addition to examining the generated output.

Contributor

wdhorton commented Aug 30, 2017

@littledan i just shuffled stuff around a bit, but I made a BigInt polyfill that wraps around the big-integer package. That part of the PR is here. In the test environment, Babel loads the polyfills, so that's what's on global.BigInt. The tests that are named exec.js are actually executing the code, so I'm using that to test that it runs, in addition to examining the generated output.

@wdhorton

This comment has been minimized.

Show comment
Hide comment
@wdhorton

wdhorton Aug 30, 2017

Contributor

@hzoo I'm seeing this error: Error: [BABEL] /home/ubuntu/babel/packages/babel-plugin-transform-bigint/test/fixtures/subtraction/subtract-number-and-string/exec.js: Cannot find module '/home/ubuntu/babel/packages/babel-plugin-transform-bigint/node_modules/babel-helper-transform-fixture-test-runner/lib/../../babel-plugin-transform-bigint' from '/home/ubuntu/babel' on the Circle tests, but I can't really figure out what's going wrong because they're passing locally. Have you seen that error before?

Contributor

wdhorton commented Aug 30, 2017

@hzoo I'm seeing this error: Error: [BABEL] /home/ubuntu/babel/packages/babel-plugin-transform-bigint/test/fixtures/subtraction/subtract-number-and-string/exec.js: Cannot find module '/home/ubuntu/babel/packages/babel-plugin-transform-bigint/node_modules/babel-helper-transform-fixture-test-runner/lib/../../babel-plugin-transform-bigint' from '/home/ubuntu/babel' on the Circle tests, but I can't really figure out what's going wrong because they're passing locally. Have you seen that error before?

@danez danez closed this Aug 31, 2017

@danez danez reopened this Aug 31, 2017

@danez danez changed the base branch from 7.0 to master Aug 31, 2017

@hzoo hzoo changed the title from BigInt (Stage 2) [WIP] to BigInt (Stage 3) [WIP] Sep 5, 2017

@undeadcat

This comment has been minimized.

Show comment
Hide comment
@undeadcat

undeadcat Sep 19, 2017

Sorry to barge in, I came here from Googling the proposal, but JFYI (or am I mistaken?) this also seems to be missing support for library functions:

BigInt.asUintN(width, BigInt)
BigInt.asIntN(width, BigInt)
BigInt.parseInt(string[, radix])

described here and in the spec.

undeadcat commented Sep 19, 2017

Sorry to barge in, I came here from Googling the proposal, but JFYI (or am I mistaken?) this also seems to be missing support for library functions:

BigInt.asUintN(width, BigInt)
BigInt.asIntN(width, BigInt)
BigInt.parseInt(string[, radix])

described here and in the spec.

@Yaffle

This comment has been minimized.

Show comment
Hide comment
@Yaffle

Yaffle commented Jun 22, 2018

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Jun 22, 2018

Member

@Yaffle that looks great. Have you tried to run it against the tests here? Would you mind helping us pushing this PR forward?

Member

xtuc commented Jun 22, 2018

@Yaffle that looks great. Have you tried to run it against the tests here? Would you mind helping us pushing this PR forward?

@Yaffle

This comment has been minimized.

Show comment
Hide comment
@Yaffle

Yaffle Jun 22, 2018

@xtuc, thanks, I didn't try to run it against tests. How can I help you with this PR?

Yaffle commented Jun 22, 2018

@xtuc, thanks, I didn't try to run it against tests. How can I help you with this PR?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jun 24, 2018

@Yaffle Great work! At a skim, this repo looks generally correct.

I'd like to argue that this transform should be kept out of all presets unless explicitly included, as it makes fairly little sense to run in production with the general performance degradation that it causes.

littledan commented Jun 24, 2018

@Yaffle Great work! At a skim, this repo looks generally correct.

I'd like to argue that this transform should be kept out of all presets unless explicitly included, as it makes fairly little sense to run in production with the general performance degradation that it causes.

@aqrln aqrln referenced this pull request Aug 10, 2018

Closed

Infinite integer, class Id #161

@TehShrike TehShrike referenced this pull request Aug 10, 2018

Open

Babel/polyfill #165

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