Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
|
Is this related to #61641 ? |
Yeah, I decided to pivot. Since full JSON materialization is likely a performance bottleneck, I’m implementing a lazy JSON parser instead. |
| test('compound root array returns document with type "array"', (t) => { | ||
| const parser = new Parser(); | ||
| const doc = parser.parse('[1, 2, 3]'); | ||
| t.assert.strictEqual(doc.root().type(), 'array'); |
There was a problem hiding this comment.
We shouldn't return string from this. This would be extremely slow. We can use an enum of numeric values called JSONType, and to .type() == JSONType.Array
| require('../common'); | ||
|
|
||
| const { test, describe } = require('node:test'); | ||
| const { Parser } = require('node:json_parser'); |
There was a problem hiding this comment.
Why limit the module name to only parser?
| const { Parser } = require('node:json_parser'); | |
| const { JSONParser } = require('node:json'); |
anonrig
left a comment
anonrig
left a comment
There was a problem hiding this comment.
- Docs are missing.
- Tests handling each edge case (errors) are missing
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
As I said in the OpenJS Slack, I don't see value in it on the core instead of a userland module.
Considering there's no internal usage of it yet, it's just another module to handle fixes and security patches.
Just curious, what’s the line of reasoning for deciding whether something should be part of the core or not? Is that documented somewhere, or is it more of a case-by-case consensus? |
|
Just to be clear: this PR isn’t ready yet. I opened it to gather feedback before moving too far ahead. |
5 participants
No description provided.