-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: remove spread of defaultOpts #72
Conversation
Just a heads up for this and future PRs, we only allow a very limited subset of conventional commit prefixes: |
61dccf8
to
028b9b8
Compare
I think this is really heading in a good direction. Let's take a very critical look at |
Just a note for either you if you pick it up or for future me if you don't. The Here's a handy diff of my notes so far. |
A note for folks who may be following along in the future, one thing that spread gives us that our new > defaults
{ sep: ' ' }
> opts
{ sep: '' }
> { ...defaults, ...opts }
{ sep: '' } Truthiness is distinct from attribute presence. HOWEVER we already have tests that show that even if you provide your own default, if it is falsey then it is coerced to a space. test('use " " as sep when opts.sep is falsey', t => {
const parsed = ssri.parse('yum-somehash foo-barbaz')
t.equal(parsed.toString({ sep: false }), 'yum-somehash foo-barbaz')
t.equal(parsed.toString({ sep: '\t' }), 'yum-somehash\tfoo-barbaz')
t.end()
}) |
028b9b8
to
8034d1b
Compare
It wasn't worth keeping this PR going for another day but heads up we'll probably move |
@wraithgar No problem, from what I remember, I use that to protect against prototype pollution and also that way is a little bit faster than the About the other perf improvements, I will try to push this week as soon I have more time. |
I was looking the CPU profiler of pnpm and I saw this call:
https://github.com/pnpm/pnpm/blob/ef6c22e129dc3d76998cee33647b70a66d1f36bf/store/cafs/src/getFilePathInCafs.ts#L29-L30
I thought about what I could do to optimize and then I found good performance improvements.
Removing spread of opts
The first thing I notice was the spread of
defaultOpts
in every method, sometimes being called twice without needing.So remove all the calls, before this change:
With the deletion of
opts
:benchmark.js