-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add @mcap/support, @mcap/nodejs, and @mcap/browser libraries #868
Conversation
I'd like to see a readme file with some documentation on how to use this stuff in the various settings. If we are going to take the time to split this out and have this library docs should be a part of that workflow. |
@@ -0,0 +1,21 @@ | |||
MIT License |
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.
fyi at least for the omgidl repo I was told that it's fine to just have a top level repo license and it doesn't need to be included in all subsequent workspaces
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 think it looks good to me. I don't have any issues with the API, the examples are clear. Only thing you might want to change is to not include test files in the build. To do this usually I have to make a tsconfig.build.js
that excludes test file endings
- Make a PR with your changes to package.json | ||
- Wait for the PR to pass CI and merge | ||
- Checkout main and tag the merged commit with `releases/typescript/{pkg}/v#.#.#` (replace #.#.# with the version you used in package.json) | ||
- Push the new tag to the repo with `git push origin releases/typescript/{pkg}/v#.#.#` |
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.
don't tag references have to be preceded by refs/tags/
?
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 think you can for disambiguation, but it's not required
Any progress on this? Making a small command-line utility for reading/writing MCAP files is currently more verbose in TypeScript than its C++ equivalent. |
It's overall in a good place but on my list of stuff to review. We have some other work that's higher priority so this hasn't gotten eyeballs yet. |
It'll still be fairly verbose with this package in its current form, but still better than it was. There are more improvements we could make that would let you basically "readMessages()" to get fully deserialized messages, but haven't decided yet whether that is going to block this merging or not. |
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 like the @mcap/node
and @mcap/browser
packages along with the decompression helpers those offer. I think that parseChannel
is too high level and specific to Studio to bundle as a one-size solution and am less keep on it depending on all the various serialization formats. Also less excited about the external maintenance that brings with it.
My opinion is that @mcap/core
readers should offer a better interface for managing message deserialization and that using parseChannel
is more unpleasant.
My take on this PR is that we should land the node and browser packages which already help usability on their own, and then look at how reading interfaces can aid with schema and message deserialization that has even less boilerplate than the parseChannel approach. Ultimately I will leave it for you to decide on which parts of this PR to merge but I would not offer parseChannel as something we should support in its current form.
@defunctzombie the parseChannel stuff has been stripped out – is this in a place you are happy with now? |
":shipit:" – said offline |
### Public-Facing Changes None ### Description Follow-up from #868 – realized 1.0.1 is a weird first version number.
…e#868) **Public-Facing Changes** - [TypeScript] Added a new package `@mcap/support`. This package provides decompression functions for zstd, lz4, and bz2, and utility functions for working with protobuf descriptors. - [TypeScript] Added a new package `@mcap/nodejs`. This package provides IReadable and IWritable implementations for Node.js `FileHandle`, reducing boilerplate for reading or writing MCAP files. - [TypeScript] Added a new package `@mcap/browser`. This package provides an IReadable implementation for the browser `Blob` (`File`) type, reducing boilerplate for reading MCAP files. - [TypeScript] Examples have been updated to use `@mcap/support` + `@mcap/nodejs` and remove repeated boilerplate **Description** **Note:** This began as un-revert of foxglove#838 (see foxglove#841) Relates to FG-2274 Resolves foxglove#719 Resolves foxglove#720 This package has been migrated from the @foxglove/mcap-support internal package in the foxglove/studio repo. Two new additions are `FileHandleWritable` under the `@mcap/nodejs` package, and `protobufDescriptors.ts` to provide helper methods for protobuf descriptors and avoid each project having to include a `protobufjs.d.ts` copy-paste. ~Studio's protobuf timestamp/duration rewriting logic has been replaced with `processRootType` & `processMessageDefinitions` functions that Studio can continue using to implement that behavior without inflicting its opinion upon other users of the library.~ Porting of deserialization for different encodings will be done in a separate PR when we have a chance to revisit the reader APIs to make them more friendly. --------- Co-authored-by: John Hurliman <[email protected]>
### Public-Facing Changes None ### Description Follow-up from foxglove#868 – realized 1.0.1 is a weird first version number.
Public-Facing Changes
@mcap/support
. This package provides decompression functions for zstd, lz4, and bz2, and utility functions for working with protobuf descriptors.@mcap/nodejs
. This package provides IReadable and IWritable implementations for Node.jsFileHandle
, reducing boilerplate for reading or writing MCAP files.@mcap/browser
. This package provides an IReadable implementation for the browserBlob
(File
) type, reducing boilerplate for reading MCAP files.@mcap/support
+@mcap/nodejs
and remove repeated boilerplateDescription
Note: This began as un-revert of #838 (see #841)
Relates to FG-2274
Resolves #719
Resolves #720
This package has been migrated from the @foxglove/mcap-support internal package in the foxglove/studio repo. Two new additions are
FileHandleWritable
under the@mcap/nodejs
package, andprotobufDescriptors.ts
to provide helper methods for protobuf descriptors and avoid each project having to include aprotobufjs.d.ts
copy-paste.Studio's protobuf timestamp/duration rewriting logic has been replaced withprocessRootType
&processMessageDefinitions
functions that Studio can continue using to implement that behavior without inflicting its opinion upon other users of the library.Porting of deserialization for different encodings will be done in a separate PR when we have a chance to revisit the reader APIs to make them more friendly.