Use modern JS features, ship TS defs

[blakeembrey]

Refactored to modernize JS features used (const, let, etc) and bump the min node.js in prep for v1. Using TypeScript to ship native .d.ts files with the package moving forward. Removes some level of input validation relying on correct types (tiny perf win, less code checks).

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@borderless/ts-scripts@0.15.0	Transitive: environment, filesystem, shell, unsafe	+122	14.9 MB	blakeembrey
npm/@vitest/coverage-v8@2.1.2	Transitive: environment, filesystem, shell	+60	9.67 MB	vitestbot
npm/typescript@5.6.2	None	0	22.4 MB	typescript-bot
npm/vitest@2.1.2	Transitive: environment, filesystem, network, shell, unsafe	+81	278 MB	antfu, oreanno, patak, ...1 more

🚮 Removed packages: npm/beautify-benchmark@0.2.4, npm/benchmark@2.1.4, npm/eslint-plugin-markdown@3.0.1, npm/eslint@8.53.0, npm/mocha@10.2.0, npm/nyc@15.1.0, npm/safe-buffer@5.2.1

View full report↗︎

[gurgunday]

Great PR!

I just have one feedback

Since this object is a map/record and can have any key, it would be beneficial and good practice to make its prototype null

Either by doing const obj = Object.create(null) or by using the more performant NullObject method (used in the Fastify router):

const NullObject = function () {}
NullObject.prototype = Object.create(null)

const obj = new NullObject();

[blakeembrey]

Awesome, thanks!

I had tried Object.create(null) but found the perf impact was -10% or so, but NullObject is comparable or better than {}.

[pi0]

Small suggestion: This line is not treeshakable by esbuild playground

You might use something like this to make sure it is side-effect free.

[blakeembrey]

Fixed in the latest commit. I'm unsure how much it helps overall since it's used in 50% of the exposed API, am I correct in understanding it's mostly for bundle size?

[blakeembrey]

The package is currently still CommonJS, so that might also waste these bytes as it won't tree shake?

[pi0]

makes sense at least until being in CommonJS (not sure if it benefits rollup build with common js)

[gurgunday]

Is there a specific reason to add the lock file?

[blakeembrey]

Not particularly, I usually work with it checked it so the package versions are consistent and I can use npm ci. I'm not strongly opinionated either way, I suppose it's possible a dependency could change their behavior and break CI for a PR 🤷

Would you prefer to keep it removed?

[gurgunday]

I don't have a strong opinion for every package but we don't use them at Fastify -- they mostly don't achieve much for consumable packages

Feel free to do as you prefer, I was just curious to see if it was intentional, sometimes people mistakenly commit it and don't realize

[wesleytodd]

We should not use them without automation which runs without the lock before each publish. The chance of end users seeing a breaking issue because we were accidentally locked back is not worth catching stray bugs during development due to updated transitive deps.