fix: allow import to be treated as Promise<Store> per the RDF/JS spec
#503
Conversation
WalkthroughThe changes introduce a new dependency for event handling in the project’s package configuration and refactor the Changes
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| stream.once('end', onEnd); | ||
| stream.once('error', onError); |
There was a problem hiding this comment.
I wanted to only create this promise when then, catch, or finally were used in the below proxy - and thus only start listening to the stream at that point in time.
Unfortunately, that turned out to not be an option; as EventEmitters error if the error event is called and there are no error listeners - which causes the last added test case to break.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/N3Store.js (3)
2-2: Check bundling approach for Node's built-in 'events' vs. external usage.The import statement uses
'events'from npm. Make sure that depending on an external package (rather than the built-in module) is intentional and required for your use case (e.g., browser builds).
414-416: Potential backpressure concerns with large streams.Calling
this.addQuad(quad)on every'data'event can be memory-intensive for large or fast streams. Consider implementing or documenting a strategy to handle backpressure (e.g., pausing the stream or using a pipeline) to avoid excessive memory usage.
417-452: Robust promise-based import logic.The promise captures the stream’s completion or error, but be mindful of any corner cases:
- If the stream is “half-closed” or partially readable, ensure data events are always consumed.
- Provide clear guidance to users on how to abort/cancel the import if needed, since the code removes listeners on completion and rejects on error.
Overall, the proxy approach linking
EventEmittermethods with the store is neatly done.test/N3Store-test.js (1)
2400-2523: Excellent test coverage for async import logic.Tests validate order of event listeners, promise resolution, error handling, and concurrency checks. Consider adding a test for partial reads or user interrupt/cancel scenarios to further strengthen reliability under edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/N3Store.js(2 hunks)test/N3Store-test.js(1 hunks)
🔇 Additional comments (2)
package.json (1)
28-28: Consider verifying necessity of external 'events' dependency.NodeJS (≥12) provides a built-in
eventsmodule, so confirm that the separately installed package is strictly required (e.g., for browser bundling). Otherwise, removing an extra dependency could simplify maintenance and reduce the bundle size.src/N3Store.js (1)
454-464: Clever proxy for returning both a promise and a stream.The proxy merges promise- and stream-like behaviors:
- This is convenient but can be confusing if users are not aware of the shape of the returned object.
- Ensure the documentation clarifies that the returned value can be awaited or used as an EventEmitter without conflict.
Otherwise, the approach looks solid.
jeswr
changed the title
Promise<Store> per the RDF/JS spec
| @@ -25,6 +25,7 @@ | |||
| ], | |||
| "dependencies": { | |||
| "buffer": "^6.0.3", | |||
| "events": "^3.3.0", | |||
There was a problem hiding this comment.
This is already a transitive dependency via readable-stream
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/N3Store.js (2)
422-423: Consider being explicit about the prototype chain delegation.While the current implementation works, being more explicit about which object receives the property access could improve clarity and maintainability.
- return Reflect.get(prop in EventEmitter.prototype ? stream : target, prop, receiver); + return prop in EventEmitter.prototype + ? Reflect.get(stream, prop, receiver) + : Reflect.get(target, prop, receiver);
414-462: Consider adding JSDoc documentation for the enhanced method.The implementation significantly changes the behavior of the
importmethod, but there's no documentation explaining its dual nature as both an EventEmitter and a Promise. Adding JSDoc comments would help users understand how to use the method correctly./** * `import` adds a stream of quads to the store + * + * @returns {EventEmitter & Promise<Store & EventEmitter>} A proxy object that acts as both an EventEmitter + * (for backward compatibility) and a Promise that resolves to the store when the stream is complete. */ import(stream) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/N3Store.js(2 hunks)
🔇 Additional comments (4)
src/N3Store.js (4)
2-2: Good addition of the EventEmitter import.This import is necessary to support the new dual-nature return type of the
importmethod, which now needs to combine event-driven functionality with promises.
414-415: LGTM: Maintained existing event handling behavior.The basic functionality of adding quads to the store as they arrive from the stream is preserved, ensuring backward compatibility.
417-449: Well-implemented Promise integration with EventEmitter.The code effectively creates a promise that resolves when the stream ends or rejects on error, while also checking for already closed streams. The design of the storeProxy elegantly combines N3Store methods with EventEmitter capabilities, allowing for a seamless dual interface.
I see from your previous comment that this implementation addresses the issue where EventEmitters error when the error event is emitted without listeners.
451-461: Excellent Proxy implementation for dual nature return type.This proxy elegantly allows the return value to be used as both a Promise and an EventEmitter without modifying the original stream. It forwards Promise methods to the promise object and all other properties to the stream, which meets the PR objective of allowing import to be treated as
Promise<Store>per the RDF/JS spec while maintaining compatibility with existing code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/N3Store.js (2)
427-427: Simplification opportunity for the proxy's get handlerThe current implementation works, but there's a slightly clearer way to express the property access logic.
- return Reflect.get(prop in EventEmitter.prototype ? stream : target, prop, receiver); + return Reflect.get(prop in EventEmitter.prototype ? stream : target, prop, receiver);This approach works well because it prioritizes EventEmitter methods from the stream while falling back to the store (target) for other properties.
432-435: Consider normalizing stream end state detectionThe current check includes multiple properties to determine if a stream has ended. While thorough, it might be slightly over-engineered.
Consider using a helper function to improve readability:
- if (stream.readableEnded || stream.destroyed || stream.readable === false || stream.closed || stream.ended) { + if (isStreamEnded(stream)) { // Resolve immediately if stream is already closed resolve(storeProxy); } + // Helper to determine if a stream has ended + function isStreamEnded(stream) { + return stream.readableEnded || stream.destroyed || + stream.readable === false || stream.closed || stream.ended; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/N3Store.js(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint (22.x)
src/N3Store.js
[failure] 415-415:
Trailing spaces not allowed
[failure] 414-414:
Trailing spaces not allowed
🪛 ESLint
src/N3Store.js
[error] 414-414: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 415-415: Trailing spaces not allowed.
(no-trailing-spaces)
🪛 GitHub Actions: CI
src/N3Store.js
[error] 414-414: ESLint: Trailing spaces not allowed. (no-trailing-spaces)
🔇 Additional comments (5)
src/N3Store.js (5)
2-2: Good addition of EventEmitter importThe inclusion of the EventEmitter import is necessary to support the new functionality where the
importmethod now returns an object that fulfills both Promise and EventEmitter interfaces.
419-421: LGTM!This section correctly sets up the event handler to add quads to the store as they arrive in the stream.
422-454: Excellent implementation of the Promise functionalityThe code creates a promise that appropriately resolves to a proxy combining the store and stream capabilities. It correctly handles early resolution for already-ended streams and properly cleans up event listeners to prevent memory leaks.
The implementation addresses the constraint mentioned in your previous comment about needing to set up error listeners immediately (rather than deferring until Promise methods are called).
456-466: Well-designed proxy implementationThis proxy elegantly allows the return value to be used as both an EventEmitter (for backwards compatibility) and a Promise (for the RDF/JS spec compatibility). The implementation selectively forwards Promise methods to the promise object while delegating all other functionality to the stream.
458-466:✅ Verification successful
Verify promise resolution is fully compatible with RDF/JS spec
The implementation treats
then,catch, andfinallyspecially for Promise compatibility. This seems correct, but it would be beneficial to verify against the RDF/JS specification that no other Promise methods are required.
🌐 Web query:
What methods must be implemented for a Promise according to the RDF/JS spec for import to return Promise<Store>?💡 Result:
To implement a Promise that complies with the RDF/JS specification for the
.import()method returningPromise<Store>, the following elements must be addressed:Promise Handling Requirements
Standard Promise Methods
The Promise must implement:
.then()for fulfillment/rejection handling.catch()for error handling (equivalent to.then(null, onRejected)).finally()for post-settlement cleanupThese methods are part of the ECMAScript specification and required for all Promises[2][6].
Stream Event Wrapping
The.import()method must wrap stream events (endanderror) into a Promise:dataset.import(stream).then(() => /* success */).catch((error) => /* handle error */)This ensures asynchronous resolution when the stream finishes or rejects on errors[9][7].
RDF/JS-Specific Implementation
Dataset Interface Compliance:
Theimportmethod is defined in theDatasetinterface as:Promise<Dataset> import(Stream stream);This requires the returned Promise to resolve to the modified dataset instance after importing all quads[9].
Quad Stream Processing:
The implementation must handle quad streams according to RDF/JS Sink/Source interfaces, where parsers/serializers implement.import()to emit quads or serialized data[1][9].Error Handling Constraints
- Rejection reasons must be
Errorinstances per W3C guidance[4].- Synchronous errors during stream processing must be converted into rejected Promises rather than thrown exceptions[4].
This structure ensures compatibility with RDF/JS datasets while adhering to JavaScript Promise standards.
Citations:
- 1: https://github.com/rdf-ext/documentation
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
- 3: https://rdf.js.org/query-spec/
- 4: https://www.w3.org/2001/tag/doc/promises-guide
- 5: https://iherman.github.io/rdfjs-c14n/
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises
- 7: https://comunica.dev/docs/query/advanced/rdfjs_querying/
- 8: https://html.spec.whatwg.org
- 9: https://rdf.js.org/dataset-spec/
- 10: General query engine interface query-spec#5
- 11: https://ruben.verborgh.org/blog/2013/04/30/lightning-fast-rdf-in-javascript/
- 12: https://rdfjs.dev/rdf-ext
- 13: Promise support #132
- 14: https://rdfjs.dev/javascript
Promise Resolution Implementation Verified as Compliant with RDF/JS Spec
- Location:
src/N3Store.js, lines 458-466.- Verification Result: The RDF/JS specification for the
import()method requires that the returned Promise implement the standard ECMAScript methods:.then(),.catch(), and.finally(). The web query confirmed that these are the only methods needed for Promise compatibility.- Conclusion: The current implementation—forwarding these methods to the underlying promise—is sufficient and meets the RDF/JS requirements. No additional Promise methods are necessary.
Uses a
Proxyso that the return ofimporthas the typeEventEmitter & Promise<Store & EventEmitter>making it compatible both with the:import(EventEmitter)import(Promise<Store>)cc @RubenVerborgh
Summary by CodeRabbit