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

Fix support for BigInt literals when using the acorn plugin #2640

Merged
merged 1 commit into from Jan 5, 2019

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Closes #2626

Description

This should address the following issues when using the acorn-bigint plugin:

  • bigint numbers wrongly being treated like "null" when checking conditionals for tree-shakeability on older environments
  • bigints being passed to Math.pow when checking conditionals on newer environments

At the moment to ensure Node 6 compatibility, "**" will be transpiled to Math.pow which does not support bigints. At the same time, the acorn plugin will wrongly mark bigint literals to have the value "null" on older systems that do not support them. This solves those issues by treating bigint literals as "unknown" on all systems. Theoretically, it would also be possible to leave them as literals on newer environments and just fix the Math.pow issue but then this would make testing much harder and I was lazy.

passing them to Math.pow or having "null" wrongly as a literal value
@lukastaegert lukastaegert merged commit be7afed into master Jan 5, 2019
@lukastaegert lukastaegert deleted the fix-big-int-support branch January 5, 2019 11:34
@danielgindi
Copy link
Contributor

Did not fix it for me (with 1.8.0)

Also adding

            acornInjectPlugins: [ require('acorn-bigint') ],

Does not help.

I'm testing with a simple "test.js" containing only const a = 10n.

@danielgindi
Copy link
Contributor

Just saw this: acornjs/acorn#824 after debugging rollup+acorn.

I had to do a hack like this:

require('acorn') && (require.cache[require.resolve('acorn')].exports = require('rollup/node_modules/acorn'));

To make acornInjectPlugins work (forcing require('acorn') to retrieve the acorn instance from rollup). But please consider adding acorn-bigint` as a builtin plugin for rollup.

@lukastaegert
Copy link
Member Author

lukastaegert commented Apr 2, 2019

Please consider adding acorn-bigint` as a builtin plugin for rollup

As it is stage 3 with a reasonable probability of becoming stage 4 soon, a PR would be accepted. Nevertheless I guess it would not hurt to show some moderate support for fixing this issue on acorn side on acornjs/acorn#824

At the moment I am not sure the maintainers recognize this as an architectural problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error in ** => Math.pow(bigInt, bigInt)
2 participants