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

Fixed issue with Buffers in config throwing error in util.makeImmutable #608

Merged
merged 1 commit into from
May 29, 2020
Merged

Fixed issue with Buffers in config throwing error in util.makeImmutable #608

merged 1 commit into from
May 29, 2020

Conversation

Ginden
Copy link
Contributor

@Ginden Ginden commented May 29, 2020

Simple code replicating issue:

config/default.js

module.exports = {foo: Buffer.from('bar', 'ascii'), bar: 5};

index.js

require('config').get('bar')

@markstos
Copy link
Collaborator

This looks good, but since Buffer values are uncommon, let's put the check towards the end the chain of checks in makeImmutable, so we aren't checking every single value to see if it's a Buffer when most of them won't be.

@Ginden
Copy link
Contributor Author

Ginden commented May 29, 2020

I don't think single instanceof check matters for performance of cold function.

On my PC checking one million elements take around 100ms.

var foo = false; 
var hrtime = process.hrtime();
for(let i = 0; i < 1000 * 1000; i++)  foo = foo || Buffer.isBuffer(String(i));

console.log(process.hrtime(hrtime))`

@markstos
Copy link
Collaborator

Ha, good enough. Merging.

@markstos markstos merged commit 7a440f9 into node-config:master May 29, 2020
@ekoeryanto
Copy link

need it soon

@lorenwest
Copy link
Collaborator

Can't believe this hasn't been published - apologies for it taking so long! Published in 3.3.2

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