Node 12 support

[kalekseev]

This is basically #122 but with reproduction steps.

Using one of the examples, but with console.log(Buffer) line in a command:

$ mkdir -p /tmp/test/rplugin/node/test

$ cat > /tmp/test/rplugin/node/test/index.js <<EOF
class MyPlugin {
  constructor(plugin) {
    this.plugin = plugin;

    plugin.registerCommand('SetMyLine', [this, this.setLine]);
  }

  setLine() {
    console.log(Buffer)
    this.plugin.nvim.setLine('A line, for your troubles');
  }
}

module.exports = (plugin) => new MyPlugin(plugin);

// Or for convenience, exporting the class itself is equivalent to the above

module.exports = MyPlugin;
EOF

On node 10 this works as expected and buffer updated with a line, on node 12 nothing happens and log contains:

Error in plugin for command:SetMyLine: Buffer is not defined (file: /tmp/test/rplugin/node/test, stack: ReferenceError: Buffer is not defined
  at MyPlugin.setLine (/tmp/test/rplugin/node/test/index.js:9:17)
  at Object.fn (/home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/NvimPlugin.js:19:26)
  at NvimPlugin.<anonymous> (/home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/NvimPlugin.js:156:41)
  at Generator.next (<anonymous>)
  at /home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/NvimPlugin.js:8:71
  at new Promise (<anonymous>)
  at __awaiter (/home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/NvimPlugin.js:4:12)
  at NvimPlugin.handleRequest (/home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/NvimPlugin.js:134:16)
  at Host.<anonymous> (/home/konstantin/.config/yarn/global/node_modules/neovim/lib/host/index.js:71:27)
  at Generator.next (<anonymous>))

[justinmk]

looks like we don't have node 12 covered in CI:
https://github.com/neovim/node-client/blob/master/.azure-templates/yarn-job.yml

PR would be a good first step.

[rhysd]

Where the global Buffer variable came from? Isn't plugin.nvim.Buffer?

[kalekseev]

Where the global Buffer variable came from? Isn't plugin.nvim.Buffer?

No, it's a global variable https://nodejs.org/api/globals.html, BTW global.Buffer contains buffer as expected.

PR would be a good first step.

PR with updated azure pipelines #142

[rhysd]

I see. Would you describe your expected behavior? I did not know the expectation.

Well, though I'm basically a maintainer of API client so I'm not so familiar with plugin-host, here is my first triage:

The plugin code is not run directly. It is run in separate context not to call dangerous functions using vm API.

This createSandbox function creates a sandboxed scope.

https://github.com/neovim/node-client/blob/master/packages/neovim/src/host/factory.ts#L104

Global variables are added by lodash.defaults(sandbox, global). However, in global variable in node, Buffer is not included as enumerated properties.

See below trial:

U'w') { node                                                                                [Corgi.local][02/18 20:20]
Welcome to Node.js v12.12.0.
Type ".help" for more information.
> var defaults = require('lodash.defaults')
undefined
> defaults({}, global)
{
  global: Object [global] {
    global: [Circular],
    clearInterval: [Function: clearInterval],
    clearTimeout: [Function: clearTimeout],
    setInterval: [Function: setInterval],
    setTimeout: [Function: setTimeout] {
      [Symbol(util.promisify.custom)]: [Function]
    },
    queueMicrotask: [Function: queueMicrotask],
    clearImmediate: [Function: clearImmediate],
    setImmediate: [Function: setImmediate] {
      [Symbol(util.promisify.custom)]: [Function]
    },
    defaults: [Function]
  },
  clearInterval: [Function: clearInterval],
  clearTimeout: [Function: clearTimeout],
  setInterval: [Function: setInterval],
  setTimeout: [Function: setTimeout] { [Symbol(util.promisify.custom)]: [Function] },
  queueMicrotask: [Function: queueMicrotask],
  clearImmediate: [Function: clearImmediate],
  setImmediate: [Function: setImmediate] {
    [Symbol(util.promisify.custom)]: [Function]
  },
  defaults: [Function]
}

I don't know why Node.js v10 worked before (I don't install old Node.js). But I guess Buffer was enumerated property in global previously.

[rhysd]

We should use Object.getOwnPropertyNames

[kalekseev]

Then the problem is this change nodejs/node#26882.
I tested lodash.defaults on node10 and it does include 'Buffer' and 'process'

[rhysd]

Thank you for the investigation. I think I can make a fix

[rhysd]

I have created PR #143 for addressing this issue.