Require node Buffer global to allow use with Webpack 5

[achingbrain]

Webpack 5 removed automatic support for node polyfills so you now can't access the node Buffer global in the browser any more - you have to require it first.

I've added Feross' Buffer polyfill as a dependency and required it in the non-cli/test files that use Buffer directly.

If running on node you'll get node's Buffer, if it's the browser you get the polyfill.

It's not worth replacing Buffer with Uint8Arrays entirely because there's no equivalent API to Buffer.allocUnsafe so it'd be a big performance hit.

[coveralls]

Coverage remained the same at 100.0% when pulling 611c8d1cd7b36114ebdd6976396f766cea360f76 on achingbrain:require-buffer into 202b8b5e59e8859e663a535c3f1a61bcd32c03fd on hildjj:master.

[hildjj]

I think we need utils and maybe process as well? There's a Webpack 5 demo in the repo now, and I got it to work by depending on node-polyfill-webpack-plugin then assuming tree shaking would sort everything out. Lazy, I know, but at least we can use that to figure out what else is needed.

In theory, I'm ok with there being some overhead for node users so that web users have a better experience, but let's make sure we've got a full solution and then see what that overhead is.

[achingbrain]

Fair point about process, yes - anything that's not required will need requireing, everything else can be configured with resolve.fallback.

[hildjj]

Also, I'd really like node users to get the native node versions, not the polyfills. I'd also like to think about testing before merging something here. Do we need to test on different browser versions? How do we merge coverage data?

[achingbrain]

Actually I think this module is ok on process as it's only used in files under /cli/*.js, I think it's fair to say those won't run in a browser.

I'd really like node users to get the native node versions, not the polyfills

If you're on node, require('buffer') will require the node core module, not the polyfill. If you're on node and want the polyfill, you have to do require('buffer/') - https://github.com/feross/buffer#usage

[achingbrain]

I'd also like to think about testing before merging something here

If you're on node there's no change so there's not much to test.

If you're in the browser you were using buffer via the webpack polyfill before v5 anyway..

[hildjj]

I've looked at this some more, and since I'm using util, which uses process without requiring it, you need do modify your webpack config anyway. Here's the minimal config I've found:

const webpack = require('webpack')
module.exports = {
  resolve: {
    fallback: {
      process: 'process/browser',
      stream: 'stream-browserify'
    }
  },

  plugins: [
    new webpack.ProvidePlugin({
      Buffer: ['buffer', 'Buffer'],
      process: 'process/browser'
    })
  ]
}

with the following dependencies added:

• buffer
• process
• stream-browserify
• util
• url

Given that, I propose that since you're going to need ProvidePlugin regardless, I doc what is needed, and reject this patch. Does that make sense @achingbrain?

[hildjj]

After thinking about this more, the Buffer part is easy to put in, has no impact on node users, and is one less thing for me to explain to web users. I'll review this patch and probably take it. I wonder what else we can do to reduce the overhead? Is there already a fork of the util package that doesn't bother with the one process.env usage? I think I can get rid of the url dependency now that we're node10+, as well.

[hildjj]

I am working up a patch for #131 as well. That's mostly-done, just needs a bit more cleanup.

[hildjj]

Apparently, the folks who make the util package don't want to fix their dependency: https://github.com/browserify/node-util/issues/43. I might just extract the one piece I need (inspect).

[hildjj]

Update: I've got a version of util.inspect extracted from the node source, which is closer to what Node 15 currently does than the node-util package, and passes most of the tests from the node test suite. I'm cleaning that up, publishing it as a new package, then I'll come back to working on the patch that includes this PR.

[hildjj]

https://github.com/hildjj/node-inspect-extracted

[hildjj]

See: https://github.com/nodejs/readable-stream/pull/414. https://github.com/nodejs/readable-stream/pull/435, https://github.com/nodejs/readable-stream/issues/450, https://github.com/nodejs/readable-stream/issues/448, etc.

We're depending on them because stream-browserify uses them. I think I might have a work-around, but it's going to take several more days.

[hildjj]

See https://github.com/hildjj/node-cbor/tree/require-buffer if you want to track my work that integrates this PR.

[hildjj]

Okay, after a marathon refactor into a real monorepo, I think I have everything working on #133, which includes this PR.

Web folks will now depend on cbor-web, and everything will mostly work. If you want bigfloat and bigdecimal support, you'll also want to depend on bignumber.js, but it should be optional.

Can I get another set of eyeballs on this, please? At least someone to download the branch, build it, and see if you can get cbor-web to work?

[hildjj]

This got swept up in #133.