-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Brim ZJSON Update #1108
Brim ZJSON Update #1108
Conversation
The zealot folder is now mainly a node typescript project. npm run build will create a bundled commonjs and es module version of the package. It will also generate a index.d.ts with all the types to carry the exported types into another typescript project. The test_api folder is a deno project and imports the bundled es module package to use and run tests agains a real zqd. The zjson file contains types for the new zjson format The zqd folder contains types for the expected payloads. The zng folder contains the classes that wrap the zjson data.
This script will run a zql search on a zqd instance that has imported sample.tsv, a zeel file with 30 records. Then the output of that search is piped to a src/js/test/responses folder that the brim app uses to mock zqd in unit tests.
The Log model and all usage has been removed The brim/record model and all usage has been removed The brim/field model has been changed to brim/cell and acts like a decorator to a zng.Field class.
I bet the tests are failing because #1109 went into the master zqd branch, but is not yet in the zjson zq branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is fantastic!! I left a few questions/comments that I don't consider to be blocking but am curious of your feedback.
name: string | ||
data: SerializedData | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put PrimitiveField
in here as well? It stands out to me to be defined within Brim instead
|
||
export default function(p = "", pins: string[] = []) { | ||
p = concatPins(p, pins) | ||
|
||
return { | ||
exclude(field: $Field) { | ||
exclude(field: Cell) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind renaming the parameter to cell as well?
field: $Field | ||
log: Log | ||
field: zng.Field | ||
log: zng.Record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment to before, mind matching the prop name to the type? log
-> record
listener(dispatch, field) { | ||
dispatch(Modal.show("whois", {addr: field.value})) | ||
listener(dispatch, data: zng.SerializedField) { | ||
const field = zng.Field.deserialize(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this new serialize/deserialize model is also what I had in mind and am using in the electron update PR 👍
if (!records.length) | ||
return Promise.reject(new CreateSubspaceError("No selected logs")) | ||
|
||
try { | ||
const logs = records.map((r) => r.mustGet("_log").stringValue()) | ||
const keys = records.map((r) => r.mustGet("key").stringValue()) | ||
const logs = records.map((r) => r.get("_log").toString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I DO like the try/get
vs get/mustGet
interface. Though I think I'd prefer we just use all of one or the other instead of both
|
||
export const openLogDetailsWindow = (): Thunk => (dispatch, getState) => { | ||
const {host, port} = mustGetConnection(getState()) | ||
const state = produce(getState(), (draft) => { | ||
for (let tab of draft.tabs.data) { | ||
delete tab.columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
After @mason-fish and I just spoke, we decided to:
|
Closes #1110
History
The zealot client has always simply passed through the raw json objects from zqd to Brim. This made the Brim code responsible for making objects to access and present the data.
In the beginning we used
models/Log.js
to represent a "record" of data. That class became outdated as we discovered the potential of our zng data model. We partially migrated tobrim/record.js
andbrim/field.js
to represent these new concepts, but all these classes were created without enough maturity to properly design them. Steve's recent changes to the zjson format and our migration to typescript have presented us an opportunity to redesign.Also, Flow and the old zjson did not allow us to properly type annotate the expected shape of the payloads.
ZQD and ZJSON Type Annotations
The zjson format and the zqd payloads are now fully typed in zealot and available in the Brim source. Access all the types like this:
zng Classes
In this PR and a new set of classes have been written to represent the zng data types in JavaScript. They are in
zealot/zng/types
and consist of:The previous two iterations (Log and brim.record) of representing the data from zqd have been removed. Zealot also constructs these objects before returning them to it's caller (Brim).
They all have a common
ZngClass
interface that looks something like this:The Record type has more methods for accessing the data within it.
Nested Records can be accessed with the get or try methods using the familiar dot syntax in the column name.
In the app now, we do not flatten the records we receive from zealot to display the main viewer. We keep them in their nested format and access them with the dot syntax. However, records do have a
.flatten()
method and it is only called in the log detail pane when displaying the table of fields.Schema
To represent a "log type" or the "schema" key in the zjson, we now have the Schema class. It's simply a wrapper around the columns array of a record. It has a
.flatten()
method. That is called to save column show/hide preferences.Field
This class is a little different from the previous
brim.field()
class. It simply attaches a name to the data.You can get at fields from a record object like so:
Serialization
Since we often need to pass these objects over ipc,
Field
,Record
, and all the other zng Classes have a serializable interface.Deno Tests
Zealot used to be a Deno project and used the Deno version of TypeScript. I learned early in this process that it's very hard to share code and types between a Deno project and a Node project. As a result:
test_api/
folder is where the integration tests with zqd are run usingdeno test
on the bundled, es module version of the zealot code located inzealot/dist/index.es.js
. (the js module ecosystem...🤯)P.S. Sorry Mason for such a big diff...again