refactor(StreamReader): Change decode_range() to return an array of objects (fixes #97).
#98
Conversation
WalkthroughThe changes update the JavaScript interop for decoded log events in the StreamReader component. Instead of returning arrays of values, the code now returns objects with named properties for each log event, and the type signature is updated accordingly in the Emscripten binding to reflect this new structure and allow for null values. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript
participant StreamReader as StreamReader (C++)
JS->>StreamReader: generic_decode_range()
StreamReader-->>JS: Array of {logEventNum, logLevel, message, timestamp} objects or null
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧠 Learnings (1)src/clp_ffi_js/ir/StreamReader.hpp (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
hoophalab
changed the title
return type to an object (fixes #97).
hoophalab
changed the title
return type to an object (fixes #97).decode_range() return type to an object (fixes #97).
hoophalab
changed the title
decode_range() return type to an object (fixes #97).decode_range() to return an array of objects (fixes #97).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
266-279: Align emitted object keys with registered TS type & docs
EM_ASMstill pusheslogEventNumber, causing the same runtime/type
mismatch noted above. Adopt the canonical names and keep alphabetical
ordering for readability:- EM_ASM( - { - Emval.toValue($0).push({ - "logEventNumber": $1, - "logLevel": $2, - "message": UTF8ToString($3), - "timestamp": $4, - }); - }, + EM_ASM( + { + Emval.toValue($0).push({ + "logEventNum": $1, + "logLevel": $2, + "message": UTF8ToString($3), + "timestamp": $4, + }); + }, results.as_handle(), - log_event_idx + 1, + log_event_idx + 1, // logEventNum log_level, - log_event_to_string(log_event).c_str(), - timestamp + log_event_to_string(log_event).c_str(), + timestamp );This resolves the contract break introduced by the rename.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/clp_ffi_js/ir/StreamReader.cpp(1 hunks)src/clp_ffi_js/ir/StreamReader.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/clp_ffi_js/ir/StreamReader.hppsrc/clp_ffi_js/ir/StreamReader.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint (ubuntu-latest)
🔇 Additional comments (1)
src/clp_ffi_js/ir/StreamReader.cpp (1)
74-75: Type string now correct – good alignment with new structureThe registered JS type matches the agreed property names and allows
null; no issues here.
|
@junhaoliao ready for another pass |
src/clp_ffi_js/ir/StreamReader.hpp
Outdated
| * - The log event's timestamp as milliseconds since the Unix epoch | ||
| * - The log event's log level as an integer that indexes into `cLogLevelNames` | ||
| * - The log event's number (1-indexed) in the stream | ||
| * @return An object representing the decoded log events. Each decoded event includes: |
There was a problem hiding this comment.
#98 (comment)
copying from above since the new comment was made from outdated code
@hoophalab to be precise, instead of returning an array of arrays, now we're returning an array of objects, so i do think it's worth mentioning "array" in the return docstring.
to make this more clear, maybe we can improve it as:
@return An array of objects, where each object represents a decoded log event with the following properties:
...
There was a problem hiding this comment.
Sorry, I didn't read it carefully. It's now ready for another pass. @junhaoliao
Description
The current
decode_range()method returns log events as arrays of 4 elements:[message, timestamp, log_level, log_event_num]. This approach has several significant drawbacks:Current issues
Poor Developer Experience: The array-based format requires developers to memorize or constantly reference documentation to understand what each index represents. This creates unnecessary cognitive overhead and increases the likelihood of bugs.
Limited Scalability: Adding new fields (e.g., Return time zone information for Unstructured IR log events. #79 ) to log events requires updating all consuming code to handle new array indices, making the API brittle and hard to evolve.
Readability: Code using the current API is less self-documenting and harder to maintain.
Change
Replace the array-based return format with structured objects that provide named properties for each log event field. This would transform:
Into:
Checklist
breaking change.
Validation performed
Test the new ffi in log viewer in this branch.
Summary by CodeRabbit