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

fix: remove unused @types/long #1785

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Aug 10, 2022

#1751 updated to v5 which ships with types. However, the Long type used here seems to be custom and not imported, so this was never used anyways?

protobuf.js/index.d.ts

Lines 1848 to 1862 in 6f0806d

/**
* Any compatible Long instance.
* This is a minimal stand-alone definition of a Long instance. The actual type is that exported by long.js.
*/
export interface Long {
/** Low bits */
low: number;
/** High bits */
high: number;
/** Whether unsigned or not */
unsigned: boolean;
}

@zmwangx
Copy link

zmwangx commented Aug 11, 2022

Just encountered the same problem. @types/long@4.0.1 is causing an additional Long type that's incompatible with the builtin Long type from long@5.0.0. Had to add import Long from "long"; to generated proto.d.ts to prevent it from picking up the wrong Long from @types/long.

@alexander-fenster alexander-fenster merged commit 0f4af83 into protobufjs:master Aug 17, 2022
@github-actions github-actions bot mentioned this pull request Aug 17, 2022
@SimenB SimenB deleted the remove-types-long branch August 17, 2022 18:09
@alexander-fenster
Copy link
Contributor

Well, this happens to be harder than I thought - adding this actually breaks some code that depended on import * as Long from 'long' being possible, so it's a breaking change. I will need to roll it back now and think how we could proceed here.

@alexander-fenster
Copy link
Contributor

Maybe not rollback, since we actually have more packages doing the same thing with long, but I'll take some time before releasing that to make sure other code I care about does not break because of this. I expect to release this some time next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants