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

Don't depend on Buffer to be compatible with Webpack 5 by default #19

Merged
merged 1 commit into from
Mar 12, 2022
Merged

Don't depend on Buffer to be compatible with Webpack 5 by default #19

merged 1 commit into from
Mar 12, 2022

Conversation

davidyuk
Copy link
Contributor

I have problems using this package in a project created by create-react-app@5.

Reproduction: https://github.com/davidyuk/reproductions/tree/cra-5-blakejs

The error I have (in browser console):

Uncaught ReferenceError: Buffer is not defined
at Object.normalizeInput (util.js:10:1)
at blake2b (blake2b.js:441:1)

It happens because Webpack 5 dropped automatic polyfilling of Nodejs modules: https://webpack.js.org/blog/2020-10-10-webpack-5-release/#automatic-nodejs-polyfills-removed. It can be fixed in webpack configuration, but it can't be changed without ejecting the react project (that some users may not want to do for simplicity).

Since node@4, Buffer inherits Uint8Array, so a separate input instanceof Buffer check is extra and I'm proposing to remove it.

Regarding the second reference to Buffer: let's use TextEncoder class, it is widely supported now (except for IE 11). If necessary it can be polyfilled on a side of the project that uses this library.

As an alternative to the proposed fix, blakejs can explicitly depend on buffer package.

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

On Node.js Buffer extended Uint8Array since forever ago, so input instanceof Buffer was no-op in practice anyway.

@nazar-pc
Copy link
Collaborator

We'll need @dcposch to publish new release though

@marc0olo
Copy link

marc0olo commented Mar 11, 2022

@nazar-pc @dcposch can we get this merged and have a new release? that would be awesome. it's quite urgent. otherwise we need to release and publish our fork on npm.

please let us know until monday

@nazar-pc nazar-pc merged commit 44fc6a3 into dcposch:master Mar 12, 2022
@nazar-pc
Copy link
Collaborator

I can merge it, but I don't have permissions to publish it to NPM.

@dcposch
Copy link
Owner

dcposch commented Mar 19, 2022

Pushing to npm.

Chang e looks great to me, thanks @nazar-pc @davidyuk

@davidyuk davidyuk deleted the fix-webpack-5 branch March 27, 2022 16:00
@davidyuk davidyuk restored the fix-webpack-5 branch March 27, 2022 16:01
@davidyuk davidyuk deleted the fix-webpack-5 branch February 21, 2023 14:16
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.

None yet

4 participants