Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

Conversation

Raynos
Copy link
Member

@Raynos Raynos commented Nov 19, 2019

Do not rely on the global.Buffer property implicitly.

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

Do not rely on the global.Buffer property implicitly.

In some environments like `electron` this global variable
does not exist at all.
@Raynos Raynos requested review from heapwolf and vweevers November 19, 2019 10:23
@Raynos
Copy link
Member Author

Raynos commented Nov 19, 2019

cc @vweevers @heapwolf

This adds electron support to the abstract-leveldown package.

@vweevers
Copy link
Member

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
Copy link
Member Author

Raynos commented Nov 19, 2019

@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
Copy link
Member

How about doing:

const window = new BrowserWindow({
  webPreferences: {
    nodeIntegration: true
  }
})

@Raynos
Copy link
Member Author

Raynos commented Nov 19, 2019

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
Copy link
Member

@Raynos
Copy link
Member Author

Raynos commented Nov 19, 2019

@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
Copy link
Member

So does #355 (comment) no longer work?

@Raynos
Copy link
Member Author

Raynos commented Nov 19, 2019

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

@vweevers
Copy link
Member

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
Copy link
Member Author

Raynos commented Nov 19, 2019

Turns out the root cause was the module v8-compile-cache ( zertosh/v8-compile-cache#3 ).

@vweevers
Copy link
Member

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
Copy link
Member Author

Raynos commented Nov 19, 2019

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

I'd still recommend merging this for hygiene.

Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point users of Webpack (which stopped polyfilling nodejs stuff) will come by asking for the same thing, so I'm cool with it.

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Nov 19, 2019
@vweevers vweevers merged commit 52d0179 into master Nov 29, 2019
@vweevers vweevers deleted the import-buffer branch November 29, 2019 09:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

semver-patch Bug fixes that are backward compatible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants