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

[types] input can be non object #17

Merged
merged 1 commit into from
Dec 21, 2024
Merged

Conversation

EdJoPaTo
Copy link
Contributor

I like seeing the addition of types and type checks by the library itself!

Given that the input can be a non-object and JSON.stringify can also have a non object input, the typings should reflect that. Especially as json-stable-stringify can be more of a drop-in replacement of JSON.stringify then. (I noticed this as updating this library resulted in type errors for me as my input could be a string in some cases)

Both JSON.stringify and @types/json-stable-stringify use any for this. Which is ok for providing typings for external code but worse for type checking the actual code with typescript. any allows doing all kinds of stuff, TypeScript just assumes that its probably ok, while unknown requires you to check the types before using them (which is done by this library). So TypeScript ensures the type checks are correct.

I also added tests that ensure that non-object input actually works both in JavaScript and have working typings (without the change to unknown TypeScript is unhappy). Not sure whether they are in the correct file. Should they get their own test file?

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

thanks! i'd caught that on the internal function but not the external one :-)

@ljharb ljharb changed the title [Types] input can be non object [types] input can be non object Dec 21, 2024
@ljharb ljharb merged commit 7135fba into ljharb:main Dec 21, 2024
363 of 365 checks passed
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.

2 participants