davidmarkclements / rfdc

Really Fast Deep Clone
MIT License
635 stars 24 forks source link

copyBuffer throws in browsers without Buffer polyfill #21

Open Jarred-Sumner opened 3 years ago

Jarred-Sumner commented 3 years ago

Currently, if you any properties cloned with rfdc are an ArrayBuffer in a browser, this exception will appear unless you have polyfilled Buffer:

Uncaught (in promise) ReferenceError: Buffer is not defined
    at copyBuffer (index.js:5)
    at clone (index.js:50)
    at PropertyPanel.tsx:196
    at mountState (react-dom.development.js:15626)
    at Object.useState (react-dom.development.js:16248)
    at Object.useState4 (react.development.js:1508)
    at PropertyPane (PropertyPanel.tsx:196)
    at renderWithHooks (react-dom.development.js:14985)
    at mountIndeterminateComponent (react-dom.development.js:17811)
    at beginWork (react-dom.development.js:19049)

It looks like the responsible code is here:

function copyBuffer (cur) {
  if (cur instanceof Buffer) {
    return Buffer.from(cur)
  }

  return new cur.constructor(cur.buffer.slice(), cur.byteOffset, cur.length)
}

There are two issues with this function:

  1. This clones the entire ArrayBuffer, when it only needs to clone the section relevant to the view. In other words, return cur.slice() would work, as TypedArray.prototype.slice will copy the section of the underlying ArrayBuffer.
  2. Since Buffer extends Uint8Array[0], you don't need to check if cur is a Buffer. .slice on a Buffer will return a Buffer

[0]: "The Buffer class is a subclass of JavaScript's Uint8Array class and extends it with methods that cover additional use cases." - https://nodejs.org/api/buffer.html#buffer_buffer

mcollina commented 3 years ago

We need the instanceof check for Node.js, not the browser. It's totally ok to have a if (global.Buffer !== undefined && cur instanceof Buffer) check.

mcollina commented 3 years ago

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

silverwind commented 3 years ago

Probably should not use global but the standardized globalThis for Buffer detection. Thought I'm not sure if it's safe to use on node < 12 which does not have globalThis.