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

BIgInt/BigFloat support #12

Open
neeboo opened this issue Aug 4, 2022 · 7 comments
Open

BIgInt/BigFloat support #12

neeboo opened this issue Aug 4, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@neeboo
Copy link

neeboo commented Aug 4, 2022

Use case

Enable BigInt/BigFloat support is important for quickjs. We have libbf.c included but CONFIG_BIGNUM is not added to build definition. Please include it as well.

Proposal

BigInt can be recognized in JavaScript side.

@neeboo neeboo added the enhancement New feature or request label Aug 4, 2022
@andycall andycall self-assigned this Aug 5, 2022
@neeboo
Copy link
Author

neeboo commented Sep 1, 2022

Any updates on this?

@neeboo
Copy link
Author

neeboo commented Sep 26, 2022

Ok, I have tried a few steps to fix the problem

  1. git checkout refactor/remove_host_class
  2. by modifying bridge/bindings/qjs/qjs_engine_patch.h, move missing types from third_party/quickjs/src/core/types.h and include <quickjs/quickjs-opcode.h> and <quickjs/libbf.h> when CONFIG_BIGNUM is on.
  3. modifying CMakeLists.txt by adding target_compile_definitions(quickjs PUBLIC CONFIG_BIGNUM CONFIG_VERSION=${\"QUICKJS_VERSION\"})

Can build dylib. but when it runs, it throws:

SyntaxError: invalid version (1 expected=2)

I found the error coming from third_party/quickjs/src/core/bytecode.c line 2027.

It seems that JS_ReadObjectAtoms function reads incorrect version of bytes value

@neeboo
Copy link
Author

neeboo commented Sep 28, 2022

Updates

Rooted problem, the polyfill scripts are compiled by node-qjsc library, which is not BIGNUM enabled. Causing byteLength is different from BC_VERSION.

See:

#define BC_VERSION BC_BASE_VERSION

I create this pull request on node-qjsc, to allow you guys consider whether we should include node-qjsc into WebF as well. Because if we need to pass "CONIFG_BIGNUM" dynamically, we should also create polyfill dynamically.

One more plugins needed to compile is the webf_websocket, it is generated by webf_cli and also needs re-compiled to BIGNUM compatible bytes.

So I strongly suggest that we should use CONFIG_BIGNUM enabled to all webf dependencies by default since most browsers have already support es2020 by default, and it is very important datatype for future apps.

Otherwise we have to rebuild all packages by ourselves to make it compatible.

Thanks again

@andycall
Copy link
Member

Thanks for your good work :) 👍🏻

I had approved your PR to node-qjsc and 0.3.0 version was shiped with BigInt support.

Now you can try to upgrade the qjsc version in polyfill to 0.3.0, and add -DCONFIG_BIGNUM in the CMakeLists.txt enable the BigInt support.

If everythings is fine, A PR is always welcome.

@BurnWW
Copy link

BurnWW commented Apr 30, 2023

@neeboo I tried to add bignum support, and the final error displayed was
SyntaxError: invalid version (1 expected=2)
What is the cause of this?
How should we continue implementing the bigint feature that was merged into the default branch?

@BurnWW
Copy link

BurnWW commented May 2, 2023

@andycall 能帮我查看一下这个问题吗, 这个对我很重要, 如果成功解决这个问题, 我想去提交这个功能的PR

@BurnWW
Copy link

BurnWW commented May 4, 2023

@andycall 我找到产生问题的原因了,node-qjsc现在只有 darwin-arm64 版本,其他平台的没有升级

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

No branches or pull requests

3 participants