Skip to content
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

Merged
merged 32 commits into from
Nov 21, 2023

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Mar 27, 2023

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 #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, 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.

@defunctzombie
Copy link
Contributor

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.

@jtbandes jtbandes changed the title Add @mcap/support library Add @mcap/nodejs and @mcap/support libraries Jul 1, 2023
@@ -0,0 +1,21 @@
MIT License
Copy link
Contributor

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

Copy link
Contributor

@snosenzo snosenzo left a 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#.#.#`
Copy link
Contributor

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/?

Copy link
Member Author

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

@jhurliman
Copy link
Contributor

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.

@defunctzombie
Copy link
Contributor

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.

@jtbandes
Copy link
Member Author

jtbandes commented Nov 2, 2023

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.

Copy link
Contributor

@defunctzombie defunctzombie left a 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.

@jtbandes
Copy link
Member Author

@defunctzombie the parseChannel stuff has been stripped out – is this in a place you are happy with now?

@jtbandes
Copy link
Member Author

":shipit:" – said offline

@jtbandes jtbandes merged commit 16bc9a8 into main Nov 21, 2023
25 checks passed
@jtbandes jtbandes deleted the jacob/support-lib branch November 21, 2023 00:21
jtbandes added a commit that referenced this pull request Nov 21, 2023
### Public-Facing Changes

None

### Description

Follow-up from #868 – realized 1.0.1 is a weird first version number.
pezy pushed a commit to pezy/mcap that referenced this pull request Jan 11, 2024
…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]>
pezy pushed a commit to pezy/mcap that referenced this pull request Jan 11, 2024
### Public-Facing Changes

None

### Description

Follow-up from foxglove#868 – realized 1.0.1 is a weird first version number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Publish Node fs IReadable/IWritable boilerplate to NPM Publish decompression handler boilerplate to NPM
4 participants