Skip to content

Add buffer import to simplify browser builds#405

Merged
mtth merged 1 commit intomtth:masterfrom
JAForbes:master
Sep 27, 2022
Merged

Add buffer import to simplify browser builds#405
mtth merged 1 commit intomtth:masterfrom
JAForbes:master

Conversation

@JAForbes
Copy link
Copy Markdown
Contributor

Fixes #403

@afzaal-ameer
Copy link
Copy Markdown

@JAForbes I haven't tested this, but I see references to both buffer and Buffer in some files. e.g. https://github.com/JAForbes/avsc/blob/52d2b93be59af6f5228599399c0881aad25e70e0/lib/index.js#L69

avsc = require('../../../../lib');

avsc = require('../../../../lib'),
{Buffer} = require('buffer');
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This syntax isn't compatible with older Node versions, could you swap to one that is? For example:

    // ...
    buffer = require('buffer');

var Buffer = buffer.Buffer;

Also, feel free to skip the files in etc/ as they are not included in the distributed package.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep no worries. Yeah I think that syntax is Node 16+.

*/

var buffer = require('buffer');
var Buffer = buffer.Buffer;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mtth I left this /etc file in, I wasn't sure if it was a polyfill for the browser and may need the explicit import.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good call, it is. I'd forgotten about that one.

Copy link
Copy Markdown
Owner

@mtth mtth left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @JAForbes.

*/

var buffer = require('buffer');
var Buffer = buffer.Buffer;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good call, it is. I'd forgotten about that one.

@mtth mtth merged commit 81170a3 into mtth:master Sep 27, 2022
@mtth
Copy link
Copy Markdown
Owner

mtth commented Sep 27, 2022

Published as 5.7.6.

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.

global Buffer usage complicates browser usage

3 participants