feat: add DOM Parser - #259
Conversation
littlespex
left a comment
littlespex
left a comment
There was a problem hiding this comment.
Blocking recommendation: I found two package/runtime regressions and two DOM-parser coverage defects that should be addressed before merging. Existing unit tests pass, but they explicitly inject the SAX parser and therefore do not exercise the new default DOM path.
The previous lockfile carried stale transitive resolutions from before the merge (rollup 4.16.4 among others), which would have downgraded the fresh lockfile on integration/v2. This regenerates from the upstream lockfile, so the only delta is sax resolving to 1.6.1 per this branch's ^1.4.1 range. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- point exports/main/types at the actual dist/main layout (dist/index.js did not exist and broke `import "imsc"`) - default fromXML() to the sax parser in node via a "node" export condition (mainNode.js); browsers keep the DOMParser default - move createSAXParser() to saxParser.js and declare sideEffects: false so browser bundles can tree-shake sax - parse from documentElement instead of firstChild so comments/PIs before the root element do not break the DOM parser - pass the parser as the fourth argument in gen-renders.js so the SAX button exercises sax rather than the DOM default - move tslib to dependencies (compiled output imports it) - guard module-scope window/document access in html.js so importing the package does not throw in node - add a regression test for the node default parser Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
palemieux
left a comment
palemieux
left a comment
There was a problem hiding this comment.
@littlespex Is mainNode.js required? main.js is needed to create a single package for web delivery.
Conflicts in package.json and package-lock.json: kept sax at ^1.6.1 from integration/v2 and kept tslib as a runtime dependency, which this branch moved out of devDependencies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
palemieux
left a comment
palemieux
left a comment
There was a problem hiding this comment.
@littlespex Since createDOMParser() is intended for use with main.js, i.e., UA runtime, can't createDOMParser() be moved there?
The library is browser-only as of v2, so the DOM parser is the only parser it ships. sax remains solely for the unit tests, which run in node, where the DOMParser global does not exist. - remove createSAXParser() from the public API and move it to the test utilities; the gen-renders webapp now uses the sax global it already loads via a script tag - remove mainNode.js and the "node" export condition; the single entry point is dist/main/main.js - move sax to devDependencies and drop @types/sax; the public Node typedef is now structural instead of referencing sax types - collapse the rollup config to one bundle pair, since the imsc.all.* variants existed only to inline sax - set "types": [] in tsconfig so builds do not depend on @types packages visible in parent directories - fix infinite recursion in "grunt clean" (self-referencing task alias) - replace DefaultParserTest with EntryPointTest, which asserts the entry imports in node and exposes exactly the public API Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| export function fromXML(xmlstring, errorHandler, metadataHandler) { | ||
| const p = sax.parser(true, { xmlns: true }); | ||
| export function fromXML(xmlstring, errorHandler, metadataHandler, parser = createDOMParser()) { |
There was a problem hiding this comment.
Is parser = createDOMParser() only intended to preserve backward compatibility?
There was a problem hiding this comment.
Yes. Originally this was implemented in a way that wouldn't introduce breaking changes, while still allowing the parser to be overridden.
There was a problem hiding this comment.
Being able to pass in a custom parser also allows for other parsers to be used, like tXml or CML's parseXml, which are considerably faster than DOMParser
There was a problem hiding this comment.
+1 to the idea of passing a parser.
I do not think we need keep backward compatibility since we are making so many changes.
What about:
- removing the default value
parser = createDOMParser()fromfromXML() - renaming
parser.jstodom_to_parser.js - moving the
Parsertypedef to its ownparser.jsmodule, and importing them from doc.js and parser.js
Should be good to merge afterwards.
There was a problem hiding this comment.
... to compete backward compatibility we could also:
- rename
fromXML(xmlstring, errorHandler, metadataHandler, parser)indoc.jstofromParser() - add
fromXML(xmlstring, errorHandler, metadataHandler, parser = createSAXParserFromDOMParser())(that callsfromParser()tomain.js
There was a problem hiding this comment.
To be clear, we want to require users to always provide the parser?
There was a problem hiding this comment.
Merged rd/minor-refactor-to-259 into this branch as-is (3287b1c), with a follow-up in 3440a14: gen-renders.js relied on the removed default and now passes createSAXParserFromDOMParser() explicitly, fromXML() reports a fatal error when called without a parser (instead of a TypeError from parser internals), and the parser JSDoc param is no longer marked optional so the emitted d.ts matches.
There was a problem hiding this comment.
Done in ae4d2f6: doc.js exports fromParser() with the parser required, and main.js exports fromXML() with the original three-argument signature defaulting to createSAXParserFromDOMParser(). fromParser() is not re-exported from the entry since four-arg fromXML() covers custom parsers; it stays importable from doc.js directly.
Review feedback from sandflow#259: - rename createDOMParser() to createSAXParserFromDOMParser(), since it creates a SAX-style event parser on top of the web platform DOMParser - remove the SAX generation paths from gen-renders, which always runs in a UA; the DOM-backed parser is used unconditionally and the sax script tag and npmcopy entry are gone sax remains a devDependency solely for the node unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
renderHTML() is browser-only (mocked or real) and now throws when browser globals are absent, per review feedback on sandflow#259. Detection moved from module scope to the first renderHTML() call, so importing the module stays safe in node (where the unit tests run) and mocked DOM globals installed after import are picked up correctly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The library is browser-only, so importing it outside a browser (mocked or real) is unsupported and may throw, per review feedback on sandflow#259. This restores html.js as it was before the environment guard and the lazy detection, and removes the test that asserted the entry point imports in node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@littlespex Many many thanks for your patience with this PR. |
Follow-up to the rd/minor-refactor-to-259 merge, which made the parser argument to fromXML() required: - gen-renders.js relied on the removed default; it now passes createSAXParserFromDOMParser() explicitly - fromXML() reports a fatal error when called without a parser, so consumers get a clear message instead of a TypeError from parser internals - the parser JSDoc param is no longer marked optional, so the emitted type declarations match the required argument Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@littlespex Thanks. Ready to merge, or do you want to take a stab at backward compatibility by renaming |
I missed your additional comment (#259 (comment)). Working on that now. |
Per review feedback on sandflow#259: doc.js now exports fromParser(), which requires a parser, and main.js exports fromXML() with the original three-argument signature, defaulting to the DOMParser-backed parser. gen-renders and the README signature are unchanged from v1 usage; tests call fromParser() directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Great. Let me know when you are done. |
|
Everything is done. Let me know if you want any other changes. |
2 participants
Resolves #242
Includes changes from #258