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

Use buffer module which support node.js and browser #31

Closed
wants to merge 4 commits into from

Conversation

xxkl1
Copy link

@xxkl1 xxkl1 commented Nov 16, 2022

The Wepack5 haven't provided polyfills for node.js core modules, and Vite does't suppot polyfill for buffer.

It will occur error on browser using the Wepack5 and Vite.

Because of the Browserify have used buffer polyfill, it is fine on browser.

I would suggest using buffer polyfill by add npm module buffer like Browserify.

link: #9 (comment)

reproduction:
Webpack5: https://stackblitz.com/edit/node-yctv93?file=src%2Findex.js,package.json
Vite: https://stackblitz.com/edit/node-bedrwr?file=package.json

hyperid.js Outdated
@@ -3,7 +3,7 @@
const uuidv4 = require('./uuid')
const parser = require('uuid-parse')
const maxInt = Math.pow(2, 31) - 1
const Buffer = require('buffer').Buffer
const Buffer = require('buffer/').Buffer
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const Buffer = require('buffer/').Buffer
const Buffer = require('buffer/').Buffer

Please use the global Buffer object if it is available

Copy link
Author

Choose a reason for hiding this comment

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

function loadBuffer () {
  const b = require('buffer')
  // use third party module if no Buffer module
  return b
    ? b.Buffer
    : require('buffer/').Buffer
}

I have added a function to load Buffer, please review my new commit if have time.

@xxkl1 xxkl1 closed this Apr 16, 2023
@DanielRuf
Copy link

Why was this closed?

@xxkl1 xxkl1 reopened this May 3, 2023
@mcollina
Copy link
Owner

mcollina commented May 3, 2023

thiw now conflicts

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Owner

mcollina commented May 3, 2023

This should really include a test

@xxkl1
Copy link
Author

xxkl1 commented May 3, 2023

This should really include a test

Let me think about how to add test for this case.

@mcollina
Copy link
Owner

closing for no activity

@mcollina mcollina closed this Apr 22, 2024
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 this pull request may close these issues.

3 participants