feat: add types declaration file with entry in package.json by geopic · Pull Request #236 · json5/json5

feat: add types declaration file with entry in package.json by geopic · Pull Request #236 · json5/json5

Resolves #235. Let me know if there are any issues.

		@@ -0,0 +1,52 @@
		declare module 'json5' {
		type StringifyOptions = Partial<{

Choose a reason for hiding this comment

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

Instead of Partial<> you could mark every entry as optonal with a ?:

replacer?: ...

Choose a reason for hiding this comment

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

That is true, but the reason I used Partial is to avoid having to label each property with a question mark since they are all optional. Do you explicitly want me to use question marks?

Choose a reason for hiding this comment

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

I have to agree that Partial feels odd here, even though it achieves the same goal. For me, Partial is used when you expect to get a subset of data you normally would get otherwise, like in a PATCH request for example. Specifying each property as optional makes more sense from a documentation standpoint I think.

Choose a reason for hiding this comment

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

Ok, no probs. I'll mark each property with a question mark. 👍

Choose a reason for hiding this comment

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

Just to return to this, Partial does label every property as optional. It's a utility type which simply serves as a shortcut in making every property of an object optional which bypasses having to add a question mark to every one of them. I'll use Partial because I assume that every property of any object structured in the StringifyOptions mold will be an optional property.

Choose a reason for hiding this comment

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

The use case for Partial is to transform an existing type into one that is a partial version of the original. Again, it feels odd here. I'd prefer to just be explicit here.

+1 Much easier to adopt when only one lib is needed for ts

		* white space is used. If white space is used, trailing commas will be used in
		* objects and arrays.
		*/
		export function stringify(value: any, replacer?: ((this: any, key: string, value: any) => any) \| (string \| number)[], space?: string \| number): string;

Choose a reason for hiding this comment

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

It's common to pass null as the replacer. All optional parameters should also except null. For parity, we should apply this to StringifyOptions too.

Choose a reason for hiding this comment

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

It is also possible to import just parse or stringify from their files in lib, but we don't have declarations for those files.

Thanks for all of your work with this. I know I'm being a stickler, but I'm just really careful with any changes I make to this project since I've declared it feature complete.

What do you think about the implementation I put together at master...jordanbtucker:typescript

If you think it looks good, I'll merge this PR and my implementation on top of it.

No worries @jordanbtucker, I know you have to get these things perfect 👌 I'll have it looked at again tomorrow or Saturday

I've looked at your implementation just now @jordanbtucker and it looks good to me. Feel free to merge and close this PR 👍

jordanbtucker changed the title ~~Add types declaration file with entry in package.json~~ feat: add types declaration file with entry in package.json

This was referenced

Feb 17, 2023