Conversation
|
Review requested:
|
|
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
There was a problem hiding this comment.
- Docs are missing.
- Tests handling each edge case (errors) are missing
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. |
In this sort of case, I'd say that it is due to the fact that the vast majority of people rely on the |
That's actually a good question, and I have been discussing with TSC a few times to clarify somewhere what makes a module prone to be on nodejs built-in and what not. Unfortunately, that's not easy to define and change from time to time - for instance, most modules that are now on Node.js built-in wouldn't be here if they were proposed 5 years ago. I concur with @jsumners-nr on it. We don't have evidence that the community needs that, nor do we have a plan to use that on Node.js internals, for instance, to improve performance somehow. Usually, modules that get inserted on Node.js built-in follow one or more of these bullet points:
Please, have in mind that more modules often mean more maintenance and more attack surface. We are currently overwhelmed with security reports, and adding a new module without matching the above bullet points will only increase the maintenance on Node.js members and the amount of security reports the security triage team needs to deal with. |
No description provided.