Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

src: Import buffer #355

Closed Raynos closed 4 years ago

Raynos commented 4 years ago

Do not rely on the global.Buffer property implicitly.

In some environments like electron this global variable does not exist at all.

Raynos commented 4 years ago

cc @vweevers @heapwolf

This adds electron support to the abstract-leveldown package.

vweevers commented 4 years ago

Hm, we test leveldown and rocksdb in Electron and have not encountered this issue. Is that because we run those tests in the main process, rather than renderer processes?

Raynos commented 4 years ago

@vweevers I believe that is correct.

This is where in electron it deletes global.Buffer ( https://github.com/electron/electron/blame/master/lib/renderer/init.ts#L188-L196 ).

vweevers commented 4 years ago

How about doing:

const window = new BrowserWindow({
  webPreferences: {
    nodeIntegration: true
  }
})
Raynos commented 4 years ago

It also looks like the root cause of my issue is very recent

https://github.com/electron/electron/pull/17838/files

It came in a PR for node12 from April this year.

vweevers commented 4 years ago

Checkout https://github.com/Level/electron-demo/blob/master/index.js

Raynos commented 4 years ago

@vweevers

The actual issue is here https://github.com/electron/electron/blame/master/lib/renderer/init.ts#L21

Module.wrapper = [
  '(function (exports, require, module, __filename, __dirname, process, global, Buffer) { ' +
  // By running the code in a new closure, it would be possible for the module
  // code to override "process" and "Buffer" with local variables.
  'return function (exports, require, module, __filename, __dirname) { ',
  '\n}.call(this, exports, require, module, __filename, __dirname); });'
]

Recent versions of electron evaluate all code in a closure that shadow global.Buffer with a function argument called Buffer whose value is always undefined

So the Buffer reference in the source code is not a global variable, it is function argument whose value is undefined ;

I guess I can try to get electron to fix this instead but not relying on global variables seems like a good thing anyway, explicit import is better then global variable reference.

vweevers commented 4 years ago

So does https://github.com/Level/abstract-leveldown/pull/355#issuecomment-555444354 no longer work?

Raynos commented 4 years ago

I can't seem to reproduce the issue in the electron-demo repository...

vweevers commented 4 years ago

Note, I'm not opposed to require('buffer') per se, just worried we'll have to do it in each and every Level module, so if it can be solved in another way, I'd prefer that.

Raynos commented 4 years ago

Turns out the root cause was the module v8-compile-cache ( https://github.com/zertosh/v8-compile-cache/issues/3 ).

vweevers commented 4 years ago

Alright. Is that module used in Electron directly (in which case I'd expect electron-demo to break when we update that to latest Electron), or by your app (in which case, there's nothing to do here, right)?

Raynos commented 4 years ago

That module is used by my application, so there's nothing to do here.

I'd still recommend merging this for hygiene.