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

feat: add types declaration file with entry in package.json #236

Merged
merged 3 commits into from Jan 31, 2021
Merged

feat: add types declaration file with entry in package.json #236

merged 3 commits into from Jan 31, 2021

Conversation

geopic
Copy link
Contributor

@geopic geopic commented Aug 12, 2020

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

lib/types.d.ts Outdated Show resolved Hide resolved
lib/types.d.ts Outdated
@@ -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?: ...

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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. 👍

Copy link
Contributor Author

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.

Copy link
Member

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.

lib/types.d.ts Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
@geopic
Copy link
Contributor Author

geopic commented Jan 27, 2021

Pushed the new changes, hope it gets merged soon 🤞 @lonewarrior556

@lonewarrior556
Copy link

lonewarrior556 commented Jan 27, 2021

+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;
Copy link
Member

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.

Copy link
Member

@jordanbtucker jordanbtucker left a comment

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.

@jordanbtucker
Copy link
Member

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.

@geopic
Copy link
Contributor Author

geopic commented Jan 29, 2021

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

@geopic
Copy link
Contributor Author

geopic commented Jan 31, 2021

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

@jordanbtucker jordanbtucker changed the title Add types declaration file with entry in package.json feat: add types declaration file with entry in package.json Jan 31, 2021
@jordanbtucker jordanbtucker merged commit eef0999 into json5:master Jan 31, 2021
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.

TypeScript type declaration file
4 participants