ljharb / json-stable-stringify

MIT License
52 stars 11 forks source link

Add get function to opts.cmp #5

Closed nikitaeverywhere closed 10 months ago

nikitaeverywhere commented 10 months ago

Resolves #4. This is a minor version bump (non-breaking change).

Other recommendations to make this package more appealing and usable in the modern world:

ljharb commented 10 months ago

Re your suggestions, TS typings belong in DT, their semver shouldn’t be conflated with those of the package itself; instanceof Array is absolutely not equivalent and should never be used; Object.keys isn’t available or correct in all supported engines; using ESM or ES5+ features would require a build process, and i don’t think there’s enough benefit to adding that here compared to the overhead.

nikitaeverywhere commented 10 months ago

All makes sense if you’re planning to maintain this lib for the already dead NodeJS v1-8 or IE. I just highlight how it looks like for someone who’s deep in NodeJS since the beginning and till now. If I were you and were willing to maintain the lib, I would do a major version bump of the lib for the current LTS node version. Those who need compatibility with 1-8 etc would use already published package versions.

For me as a package user, for instance, this matters:

You also don’t need typescript and a build pipeline (but having in-house .d.ts declaration would be just great). Nowadays you can write a «type: module» pure ES module lib in modular JavaScript. Which will work in all LTS NodeJS versions.

On Thu, 9 Nov 2023 at 17:53, Jordan Harband @.***> wrote:

Re your suggestions, TS typings belong in DT, their semver shouldn’t be conflated with those of the package itself; instanceof Array is absolutely not equivalent and should never be used; Object.keys isn’t available or correct in all supported engines; using ESM or ES5+ features would require a build process, and i don’t think there’s enough benefit to adding that here compared to the overhead.

— Reply to this email directly, view it on GitHub https://github.com/ljharb/json-stable-stringify/pull/5#issuecomment-1804198251, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGCCSFPSRBNB6SYABNEN5TYDUDBLAVCNFSM6AAAAAA7EPG6DCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGE4TQMRVGE . You are receiving this because you authored the thread.Message ID: @.***>

ljharb commented 10 months ago

Yes, i plan on supporting down to node 0.4 and associated browsers forever.

even if i weren’t, though, i wouldn’t use native ESM, since it’s worse; and even if i did, I’d never use type module, I’d use .mjs. I’m quite familiar with how it works; i was on the node modules working group and I’m on the language spec committee.

Seperately, more deps is better - zero deps almost certainly means that correctness and robustness has been sacrificed in favor of reinventing wheels. Bundle size is inflated by copy pasted code, not deduped packages.

nikitaeverywhere commented 10 months ago

Fair. I 💜 NodeJS

On Thu, 9 Nov 2023 at 21:56, Jordan Harband @.***> wrote:

Yes, i plan on supporting down to node 0.4 and associated browsers forever.

even if i weren’t, though, i wouldn’t use native ESM, since it’s worse; and even if i did, I’d never use type module, I’d use .mjs. I’m quite familiar with how it works; i was on the node modules working group and I’m on the language spec committee.

Seperately, more deps is better - zero deps almost certainly means that correctness and robustness has been sacrificed in favor of reinventing wheels. Bundle size is inflated by copy pasted code, not deduped packages.

— Reply to this email directly, view it on GitHub https://github.com/ljharb/json-stable-stringify/pull/5#issuecomment-1804673456, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGCCSHMXQJZ3T6GJNXXYH3YDU7QLAVCNFSM6AAAAAA7EPG6DCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGY3TGNBVGY . You are receiving this because you authored the thread.Message ID: @.***>

ljharb commented 10 months ago

I've rebased this, but what are your thoughts on https://github.com/ljharb/json-stable-stringify/pull/5#discussion_r1388285361 ?

nikitaeverywhere commented 10 months ago

I've rebased this, but what are your thoughts on https://github.com/ljharb/json-stable-stringify/pull/5#discussion_r1388285361 ?

Thank you, looks good, please commit. I didn't know __proto__: null would optimize anything.

ljharb commented 10 months ago

Oh that’s not the optimization part, that’s just for safety :-) the optimization part would be not creating the get function when it’s not needed.

nikitaeverywhere commented 10 months ago

Thanks Jordan!