LiteBuffer overwrites Buffer import in React Native

[lachenmayer]

Hey there, thanks a lot for this module. We're using this inside a React Native app with the WebSocket client, which apart from a few rough edges has been a real pleasure. I just updated the rsocket-* dependencies to 0.0.25, and I started to get errors in unrelated parts of our app.

We're using the buffer polyfill in a bunch of places to be able to share code between Node.js and React Native.

In particular, we are getting the following error: Not implemented: "hex" encoding when trying to call buf.toString('hex'). We are using this in a generic hash function to generate checksums.

The root cause of this is the following in LiteBuffer:

https://github.com/rsocket/rsocket-js/blob/master/packages/rsocket-core/src/LiteBuffer.js#L1003

  if (hasBufferModule) {
    // ExistingBuffer is likely to be a polyfill, hence we can override it
    // eslint-disable-next-line no-undef
    // $FlowFixMe
    Object.defineProperty(ExistingBufferModule, 'Buffer', {
      configurable: true,
      enumerable: false,
      value: Buffer,
      writable: true,
    });
  }

I'm not sure I understand the reasoning behind this - why can't the code inside the rsocket library that depends on this LiteBuffer class import LiteBuffer? Or otherwise, why can't RSocket use existing buffer implementations available in the environment?

In general, I'd like to hear your rationale for reimplementing Buffer. The buffer library is one of the most widely used libraries on NPM, and in terms of performance is fairly close to the Node.js implementation for most operations. If you need efficient zero-copy appends, I recommend checking out the bl module which creates a view over several buffers, without copying them (unless necessary).

Expected Behavior

I expect this module to not mess with any imports outside of the module, or any globals.

Actual Behavior

When importing buffer after RSocket like this:

import { RSocketClient } from 'rsocket-core'
// ... possibly in another file...
import { Buffer } from 'buffer'

The Buffer value is now actually LiteBuffer, which doesn't implement a lot of the expected Buffer APIs.

Steps to Reproduce

I hope this is clear enough to not need a repro case, can write one if this isn't the case.

Possible Solution

Either ensure that all code relying on Buffer inside the rsocket repo instead imports LiteBuffer, or try to make LiteBuffer act like a polyfill in the sense that it should use global.Buffer if it exists, attempt to import from buffer, and if both of those fail, create global.Buffer.

Your Environment

• RSocket version(s) used: 0.0.25
• Other relevant libraries versions (eg. netty, ...): buffer v6.0.3
• Platform (eg. JVM version (javar -version) or Node version (node --version)): React Native 0.63.4
• OS and version (eg uname -a): any

Thanks for your time, let me know if I can provide any more info!

[OlegDokuka]

Hi @lachenmayer!

Thank you for reporting that! We know about that problem and will tackle it in 0.1.x or 1.0.x releases please see #112 for more info.

Unfortunately, having a polyfill module on the path does not always mean having it imported globally (e.g. available at window). That said, at the moment, the best we can do is to force override the polyfill.

What I can recommend doing to mitigate the problem is kind of doing the same once the LiteBuffer class has installed itself to the window. Once that happened, you may override it again with the polyfill (hack but can be a workaround).

Meanwhile, I will add support for hex string decoding so that will not be a blocker for you as of now

[OlegDokuka]

Alternatively, I may avoid doing a hard override of the polyfill module, but not sure if it solves the problem, @lachenmayer can you please check if removing the following solves the problem

if (hasBufferModule) {
    // ExistingBuffer is likely to be a polyfill, hence we can override it
    // eslint-disable-next-line no-undef
    // $FlowFixMe
    Object.defineProperty(ExistingBufferModule, 'Buffer', {
      configurable: true,
      enumerable: false,
      value: Buffer,
      writable: true,
    });
  }

[lachenmayer]

Hey Oleg, thanks so much for the quick reply! We're working around it for now by ensuring that we're setting global.Buffer = Buffer in our app before we do anything else (so actually properly polyfilling rather than just importing). This works around this issue for now.

I have tested removing that clause, and that works for us too (we're going to go with the polyfill solution as that's less fragile for now). Short term, I think it could be a good solution to remove that. I think it's a fair assumption that if you can import buffer that it actually points to a sensible buffer implementation, and falling back to LiteBuffer only if that fails completely.

Separating LiteBuffer into a separate package seems like a sensible solution long-term, thanks again for your help.

[OlegDokuka]

@lachenmayer Thank you for the quick reply as well! I will remove the mentioned block, so we will not be overriding polyfill anymore