Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer detection #7

Closed
achingbrain opened this issue Mar 24, 2021 · 3 comments · Fixed by #12
Closed

Buffer detection #7

achingbrain opened this issue Mar 24, 2021 · 3 comments · Fixed by #12

Comments

@achingbrain
Copy link
Contributor

I'm trying to build this module with webpack in js-ipfs/examples/browser-webpack and the Buffer detection doesn't work:

ERROR in ../../node_modules/cborg/esm/lib/byte-utils.js 1:32-39
Module not found: Error: Can't resolve 'process/browser' in '/Users/alex/Documents/Workspaces/ipfs/js-ipfs/node_modules/cborg/esm/lib'
Did you mean 'browser.js'?
BREAKING CHANGE: The request 'process/browser' failed to resolve only because it was resolved as fully specified
(probably because the origin is a '*.mjs' file or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.
 @ ../../node_modules/cborg/esm/lib/encode.js 9:0-40 192:13-18
 @ ../../node_modules/cborg/esm/cborg.js 1:0-41 7:0-12:2
 @ ../../packages/ipfs-core/src/components/pin/pin-manager.js
 @ ../../packages/ipfs-core/src/components/index.js 93:20-48
 @ ../../packages/ipfs-core/src/index.js 27:4-27
 @ ../../packages/ipfs/src/index.js 12:35-55
 @ ./src/components/app.js 11:11-26
 @ ./src/components/index.js 7:10-26

It's good to use buffer because Buffer.allocUnsafe(size) is significantly faster than new Uint8Array(size), but it's bad to use node globals.

Solutions:

  1. Use the process module:
import process from 'process'

export const useBuffer = !process.browser && global.Buffer && typeof global.Buffer.isBuffer === 'function';
  1. Use the buffer module:
import { Buffer } from 'buffer'

export const useBuffer = true;

// replace global.Buffer with Buffer
achingbrain referenced this issue in achingbrain/cborg Mar 25, 2021
We can't use the `**` operator as right now it gets transpiled to `Math.pow`
which only takes Numbers.

Similarly, trying to avoid referencing node globals to get a `Buffer` instance
causes similar problems now that bundlers don't tend to come with node polyfills
any more.

Swaps the use of `**` for the BigInt literal result of the calculation and uses
the `Buffer` module to ensure compatibility.

Fixes #7
Fixes #8

Please publish with a `main` field to also fix #9.
@rvagg
Copy link
Owner

rvagg commented Mar 26, 2021

What I don't understand about this is that I test this, in the package scripts and in GitHub Actions with Webpack 4 with standard polyfill settings, and I had a user contribute support for 5 without any polyfills. It's not supposed to break with Webpack of all the bundlers! I've gone to a lot of effort here to avoid triggering pulling in the polyfills at all for Webpack 4 even when they're available.

@achingbrain
Copy link
Contributor Author

achingbrain commented Mar 26, 2021

A fix attempt was made in #11 but this is still broken under certain conditions, notably bundling with webpack 5.

cborg.js_+_16_modules:134 Uncaught ReferenceError: global is not defined
    at eval (cborg.js_+_16_modules:134)
    at Module.19603 (bundle.js:1)
    at __webpack_require__ (bundle.js:1)
    at eval (pin-manager.js:11)
    at Object.78773 (bundle.js:1)
    at __webpack_require__ (bundle.js:1)
    at eval (add-all.js:5)
    at Object.23164 (bundle.js:1)
    at __webpack_require__ (bundle.js:1)
    at eval (index.js:4)

@achingbrain
Copy link
Contributor Author

Swapping out global for globalThis is enough to fix this.

achingbrain referenced this issue in achingbrain/cborg Mar 26, 2021
`global` is not defined in browsers.  Use `globalThis` instead.

Fixes #7
@rvagg rvagg closed this as completed in #12 Mar 27, 2021
rvagg pushed a commit that referenced this issue Mar 27, 2021
`global` is not defined in browsers.  Use `globalThis` instead.

Fixes #7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants