fix: handle big integers in incoming events#495
Conversation
src/message/http/index.ts
Outdated
| */ | ||
|
|
||
| import { types } from "util"; | ||
| import JSON from "json-bigint"; |
There was a problem hiding this comment.
Have you measured the impact of using this library versus using a native V8 implementation?
If the difference is significant (it might be), we should consider gating this behavior with a configuration option (it may be turned on by default IMHO).
There was a problem hiding this comment.
This is a good point, so I decided to try a simple, probably naive test. But no matter how naive, you can't ignore a 7x slowdown with bigint support.
❯ time node index.js
Unsing native JSON
....................................................................................................
node index.js 199.67s user 1.29s system 98% cpu 3:23.76 total
❯ time node index.js bigint
Using bigint
....................................................................................................
node index.js bigint 1308.75s user 4.49s system 99% cpu 21:55.06 total
❯ cat index.js
1 │ const fs = require('fs');
2 │
3 │ const J = require('json-bigint')({ useNativeBitInt: true });
4 │ let Parser;
5 │
6 │ if (process.argv[2] === 'bigint') {
7 │ Parser = J;
8 │ console.log('Using bigint');
9 │ } else {
10 │ Parser = JSON;
11 │ console.log('Unsing native JSON');
12 │ }
13 │
14 │ const json = fs.readFileSync('./generated.json');
15 │
16 │ let remain = 10_000_000;
17 │ while (remain-- > 0) {
18 │ Parser.parse(json);
19 │ if (remain%100000 === 0) {
20 │ process.stdout.write('.');
21 │ }
22 │ }
23 │
24 │ console.log();
There is no obvious place to set global parser variables. I need to think on this a bit.
There was a problem hiding this comment.
@lance Have you thought of where to place this feature flag?
|
Closing and reopening to trigger re-run of CI with updated package.json (to hopefully fix errors) |
|
This pull request is stale because it has been open 30 days with no activity. |
An event may have data that contains a BigInt. The builtin `JSON` parser for JavaScript does not handle the `BigInt` types. The introduced `json-bigint` dependency (34k) does. Fixes: cloudevents#489 Signed-off-by: Lance Ball <lball@redhat.com>
|
@cardil I have added an environment variable to enable the @lholmquist ptal |
Signed-off-by: Lance Ball <lball@redhat.com>
lholmquist
left a comment
There was a problem hiding this comment.
/lgtm.
I do like your suggestion of adding this to the method signature, which as you said is a bigger change, but maybe we can do that later then
An event may have data that contains a BigInt. The builtin
JSONparserfor JavaScript does not handle the
BigInttypes. The introducedjson-bigintdependency (34k) does.Fixes: #489
Signed-off-by: Lance Ball lball@redhat.com