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

Remove lodash dependency from @mcap/core #597

Merged
merged 3 commits into from
Sep 22, 2022
Merged

Conversation

jhurliman
Copy link
Contributor

@jhurliman jhurliman commented Sep 22, 2022

Public-Facing Changes

  • TypeScript library @mcap/core can be used without manually installing lodash

Description
lodash was used in a few places but there was no dependency on it in package.json. This replaces the few uses of lodash generic methods with more optimized functions tuned for the @mcap/core library. In particular, it avoids heap allocation of new string objects during binary searches.

Fixes #588

@jhurliman jhurliman requested review from defunctzombie and jtbandes and removed request for defunctzombie September 22, 2022 01:48
a.messageEncoding === b.messageEncoding &&
a.schemaId === b.schemaId &&
a.topic === b.topic &&
a.metadata.size === b.metadata.size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we can do to protect against adding fields but forgetting to define them here for equality comparison?

Random thought: might it be better to store a crc of the original record bytes and compare that?

@@ -191,3 +189,14 @@ export function parseRecord(
throw new Error("Not yet implemented");
}
}

function isChannelInfoEqual(a: ChannelInfo, b: ChannelInfo): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thoughts to the channel info comparison below - maybe a crc on the bytes would be effective and work if we ever add a new field.

defunctzombie
defunctzombie previously approved these changes Sep 22, 2022
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.

👍 to dropping lodash. Less thrilled about the untested sortedIndexBy. This is the type of method we should have tests for and why we use libraries that have these methods tested.

@defunctzombie defunctzombie dismissed their stale review September 22, 2022 02:49

Meant to add as a comment rather than approve so Jacob could still review

@jhurliman jhurliman merged commit f633bb2 into main Sep 22, 2022
@jhurliman jhurliman deleted the jhurliman/typescript-nodash branch September 22, 2022 18:55
});

it("handles negation", () => {
const array: [bigint, bigint][] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems invalid? The input array is not sorted by (x)=>-x.

const computedValue = iteratee(value);

while (low < high) {
const mid = (low + high) >>> 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other math adjustment I was referring to was low + (high - low) >>>1, to avoid overflow during addition

jtbandes added a commit to foxglove/ws-protocol that referenced this pull request Nov 14, 2022
**Public-Facing Changes**
Upgrades @mcap/core to get the fix from foxglove/mcap#597
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.

Typescript mcap library requires lodash but does not depend on it
3 participants