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

Writer.create unintended breaking change due to Buffer support #1791

Open
skeet70 opened this issue Aug 11, 2022 · 1 comment
Open

Writer.create unintended breaking change due to Buffer support #1791

skeet70 opened this issue Aug 11, 2022 · 1 comment

Comments

@skeet70
Copy link

skeet70 commented Aug 11, 2022

protobuf.js version: 7.0.0

I'm upgrading from 6.11.3 to 7.0.0. I have a test that calls

/**
 * Take the provided information and encode it into our EDEK protobuf structure.
 */
export const encodeEdeks = (segmentId: number, documentId: string, userKeys: EncryptedAccessKey[], groupKeys: EncryptedAccessKey[]): Uint8Array => {
    const userDeks = userKeys.map(convertEncryptedAccessKey("userId"));
    const groupDeks = groupKeys.map(convertEncryptedAccessKey("groupId"));
    // TODO: types say this should return a Uint8Array but it's actually returning a Buffer. Nothing in the changlog
    // indicates this should be happening going from 6.11.3 to 7.0.0, but testing indicates it is.
    const x = PBEDeks.encode(new PBEDeks({edeks: [...groupDeks, ...userDeks], segmentId, documentId})).finish();
    console.log(x);
    return x;
};

and expects any<Uint8Array> back. That test is failing after upgrading because it's now getting a Buffer back. I believe this is an un-communicated breaking change due to the buffer configuration support (add support for buffer configuration (#1372) (101aa1a)). In my case the code keeps working in the browser (where I don't have Buffer polyfilled) but fails in Jest tests (which do have a Buffer lying around). There could be other non-test situations where this would break people's current code.

The reader-writer example run in an environment where Buffer is available but not expected/desired would reproduce the issue.

Desired outcome for me would be either:

  • document the possibly breaking change in the "Breaking Changes" list for 7.0.0 or,
  • make Writer explicitly just Uint8Array and those wanting to use Buffer use BufferWriter with a create that also explicitly uses/expects Buffer. If someone wants the implicit behavior they could create their own FlexibleWriter (or it could be provided) that has the ternary on util.Buffer in the create to make one of the two explicit ones.
@skeet70
Copy link
Author

skeet70 commented Aug 11, 2022

Confirmed that guarding against it in my tests works. So the workaround is easy, but it'll likely be hard for people upgrading to detect that this is the issue (especially if it's not called out as potentially breaking).

if (Buffer) {
    return new Uint8Array((encodedEdeks as Buffer).buffer);
} else {
    return encodedEdeks;
}

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

No branches or pull requests

1 participant