webf icon indicating copy to clipboard operation
webf copied to clipboard

BIgInt/BigFloat support

Open neeboo opened this issue 3 years ago • 7 comments
trafficstars

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 avatar Aug 04 '22 19:08 neeboo

Any updates on this?

neeboo avatar Sep 01 '22 02:09 neeboo

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 avatar Sep 26 '22 14:09 neeboo

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:

https://github.com/openwebf/webf/blob/80f32ab9856c7d69ef1ff778e083a323f4f83f51/bridge/third_party/quickjs/src/core/bytecode.c#L149

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

neeboo avatar Sep 28 '22 23:09 neeboo

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.

andycall avatar Sep 29 '22 01:09 andycall

@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 avatar Apr 30 '23 00:04 BurnWW

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

BurnWW avatar May 02 '23 23:05 BurnWW

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

BurnWW avatar May 04 '23 07:05 BurnWW