Add TypeScript definition and require Node.js 6 #17

DenisCarriere · 2018-08-20T19:25:45Z

Add TypeScript definition to write-json-file

Ref: https://github.com/sindresorhus/typescript-definition-style-guide
sindresorhus/ama#439 (comment)

sindresorhus · 2018-08-20T19:43:34Z

Thanks for creating a type definition 🙌

DenisCarriere · 2018-08-20T19:50:15Z

@sindresorhus You're welcome 😄 I've been looking forward to add Typescript definitions to your repos for a while.

Glad you've started accepting them 👍

DenisCarriere · 2018-08-20T19:52:59Z

Still getting a few errors when running tests, not too sure if these are valid errors. 🤔

xo doesn't seem to handle Typescript definitions very well
prefer-destructuring doesn't seem like it's possible to deconstruct on line 52

CC: @sindresorhus

$ npm t

> write-json-file@2.3.0 test /Users/denis/Github/write-json-file
> xo && ava


  index.d.ts:1:6
  ✖   1:6  Parsing error: Unexpected token Replacer

  index.js:52:4
  ✖  52:4  Use object destructuring.                 prefer-destructuring

  2 errors
npm ERR! Test failed.  See above for more details.

sindresorhus · 2018-08-20T19:48:23Z

index.d.ts

+	indent?: string | number | null;
+	detectIndent?: boolean;
+	sortKeys?: boolean | ((a: string, b: string) => number);
+	replacer?: Replacer | Array<number | string> | null;


This should not be null.

sindresorhus · 2018-08-20T19:49:30Z

index.js

+module.exports.default = (fp, data, opts) => {
+	return makeDir(path.dirname(fp), {fs})
+		.then(() => init(main, fp, data, opts));
+};


Instead of duplicating the code here, assign the method to a variable and then assign that variable to both module.exports and module.exports.default.

👍 makes sense

sindresorhus · 2018-08-20T19:50:01Z

package.json

  ],
+  "types": "index.d.ts",


This is not needed. From the style guide:

You don't need to add a typings field to package.json as TypeScript will infer it from the name.

Sounds good, didn't know this wasn't required, good to know.

sindresorhus · 2018-08-20T19:53:04Z

index.d.ts

+}
+
+/**
+ * Stringify and write JSON to a file atomically


Should end in a dot.

sindresorhus · 2018-08-20T19:53:47Z

index.d.ts

+
+interface Options {
+	indent?: string | number | null;
+	detectIndent?: boolean;


Is there any way to define default arguments in the types themselves instead of in the docs?

Good question, I don't think it's possible as an interface.

[ts] A parameter initializer is only allowed in a function or constructor implementation.

You can do it if the types are created from a .ts file

sindresorhus · 2018-08-20T19:57:44Z

index.d.ts

+ * @param {boolean} [options.detectIndent=false] Detect indentation automatically if the file exists.
+ * @param {boolean|function} [options.sortKeys=false] Sort the keys recursively. Optionally pass in a compare function.
+ * @param {function|Array<number|string>|null} [options.replacer] Passed into JSON.stringify.
+ * @param {number} [options.mode=0o666] Mode used when writing the file.


From the style guide:

@param should not include the parameter type.

Since it's inferred from the type definition already and just results in duplication.

sindresorhus · 2018-08-20T19:58:15Z

index.d.ts

+ * @param {number} [options.mode=0o666] Mode used when writing the file.
+ * @returns {void}
+ * @example
+ * const writeJsonFile = require('write-json-file');


Use import

sindresorhus · 2018-08-20T19:58:58Z

index.d.ts

+ *
+ * writeJsonFile('foo.json', {foo: true}).then(() => {
+ * 	console.log('done');
+ * });


Use this instead:

(async () => { await writeJsonFile('foo.json', {foo: true}); console.log('done'); })();

👍 I like this example as well

sindresorhus · 2018-08-20T20:01:06Z

index.d.ts

+ *
+ * @param {string} filepath Filepath
+ * @param {any} data Data
+ * @param {object} [options] Optional parameters


No need to have a description for these 3 parameters as the name is clear enough.

Fair enough, dropping those params can be extracted from the Types

sindresorhus · 2018-08-20T20:06:50Z

xo doesn't seem to handle Typescript definitions very well

Yeah... You have to exclude the type definition file from being linted because of xojs/xo#348. Example.

sindresorhus · 2018-08-20T20:08:52Z

I'm also looking for feedback on the style guide. Anything that could be clearer/improved? Anything you strongly disagree with? You should probably read it again as I have updated it after reviewing this PR.

sindresorhus · 2018-08-20T20:31:26Z

package.json

  },
  "scripts": {
-    "test": "xo && ava"
+    "test": "xo --ignore index.d.ts && ava"


Do it like the example is shared: https://github.com/sindresorhus/delay/blob/c17e29b13c20e6be35d64d75f45ce174bb793158/package.json#L48-L52

👍 Sounds good!

sindresorhus · 2018-08-20T20:31:50Z

index.d.ts

+export function sync(filepath: string, data: any, options?: Options): void;
+
+/**
+ * Stringify and write JSON to a file atomically


Should end in a dot.

sindresorhus · 2018-08-20T20:32:31Z

index.d.ts

+ *
+ * Creates directories for you as needed.
+ *
+ * @returns {Promise<void>}


This is moot as it has no description and therefore is better documented by the type definition itself.

👍 yea it's pretty pointless, it is defined by the Types

sindresorhus · 2018-08-20T20:32:53Z

index.d.ts

+ *
+ * Creates directories for you as needed.
+ *
+ * @returns {void}


Same as https://github.com/sindresorhus/write-json-file/pull/17/files#r211397928

sindresorhus · 2018-08-20T20:33:18Z

index.d.ts

+ *
+ * @returns {void}
+ * @example
+ * import * as writeJsonFile from 'write-json-file';


Should be import writeJsonFile from 'write-json-file';

🤔 ... to access .sync() you need to add import * as

Ah, good point 👍

I guess if you use Babel this example is correct, I'll change it to:

import writeJsonFile from 'write-json-file';

Most Typescript users kinda got used to adding import * as, no need to include it.

Most docs are mixed between Babel & Typescript, we can keep as the Babel way

sindresorhus · 2018-08-20T20:36:09Z

index.d.ts

+ * import writeJsonFile from 'write-json-file';
+ *
+ * (async () => {
+ * 		await writeJsonFile('foo.json', {foo: true});


Incorrect indent

🤦‍♂️

sindresorhus · 2018-08-20T20:37:19Z

Need feedback on: #17 (comment)

Is there any way to define default arguments in the types themselves instead of in the docs?

We need to define default values somewhere for improved auto-completion experience.

sindresorhus · 2018-08-20T20:42:48Z

// @brandon93s @NeekSandhu Would you be able to help review this?

DenisCarriere · 2018-08-20T21:13:45Z

@sindresorhus I've looked over all your comments and I hope we've covered everything 😄

Got another PR sindresorhus/load-json-file#15 with the same changes.

I'm done for commits, unless you have more comments and reviews.

Let me know if you come up with a solution for #17 (comment)

brandon93s · 2018-08-21T02:30:07Z

Looks good to me.

For #17 (comment), unfortunately I don't think there is a good way for defaults to be defined on an interface. Adding the default to the interface documentation would at least make it visible in the intellisense:

/**
 * Detect indentation automatically if the file exists.
 * 
 * Default: false
 */
detectIndent?: boolean;

brandon93s · 2018-08-21T02:40:00Z

index.d.ts

+ * writeJsonFile.sync('foo.json', {foo: true});
+ * console.log('done');
+ */
+export function sync(filepath: string, data: any, options?: Options): void;


data can't really be any. For example, passing in undefined will throw a TypeError. We could attempt to express valid JSON values to be more specific here.

Consider the fact that even in TypeScripts "official" lib.d.ts file, the signature for JSON.stringify( ... ) is

JSON.stringify(value: any, replacer?: (key: string, value: any) => any, ...): string;

That means even the TypeScript team couldn't come with a reliable, sound way to describe JSON.

There's always the object type, however I kinda like using any for this param.

@brandon93s was hoping to prevent passing undefined (which causes TypeError), the problem is that no signature of any kind (as of ts@3.0.1) can prevent that from happening.

function foo(a: object | number | string): void { } // ts won't complain i.e no red squiggly lines foo(undefined) // no complains here either foo(null)

EDIT

I was wrong, I forgot about strictNullChecks option that TypeScript has for enforcing this. So in theory data: object | string | number is better than data: any. It'll remind the user of bad input as long as they have strictNullChecks options engaged.

Yeah, let's make it strict. Can be a type called JSONStringifyable maybe.

👍 for making it more strict in the data param.

There's two options:

We create a type called JSONStringifyable (the type is slightly "hidden" when using intellisense)

We add the types directly in the data param, that way it's clear that the input types are string|object|number (my preferred choice).

SamVerschueren · 2018-08-21T03:40:42Z

package.json

-  }
+  },
+  "xo": {
+		"ignores": [


The indentation is off. These lines are using tabs and the others are using spaces.

I know @sindresorhus switched to tabs for package.json because npm preserves them nowadays. But think it should be done in a separate commit.

zikaari · 2018-08-21T03:45:55Z

As far as #17 (comment) goes, like @brandon93s said, unfortunately not possible yet. I've been using the method he advised, i.e to describe the default value in comment(s), works really well IMO.

Future commit will convert package.json to tabs

DenisCarriere · 2018-08-21T04:54:34Z

Added defaults using the Default: '\t' schema in the comments.

sindresorhus · 2018-08-21T07:40:56Z

According to https://twitter.com/felixfbecker/status/1031660800413454337, we can use the @default tag, so let's do that.

zikaari · 2018-08-21T09:29:39Z

wow...totally TIL thing. It's even worth mentioning in your style guide.

SamVerschueren · 2018-08-21T09:37:51Z

.gitignore

@@ -1 +1,2 @@
 node_modules
+yarn.lock


Add newline at the end of the file.

Thanks for catching this 👍

Added newline

zikaari · 2018-08-21T09:54:36Z

While the context is heated up, have you considered sindresorhus/ama#554

Ref: http://usejsdoc.org/tags-default.html

DenisCarriere · 2018-08-21T13:36:04Z

@SamVerschueren @sindresorhus

Added new line to .gitignore
Using @default tags (ref: http://usejsdoc.org/tags-default.html)

DenisCarriere · 2018-08-21T13:41:55Z

index.d.ts

+ * Creates directories for you as needed.
+ *
+ * @example
+ * import * as writeJsonFile from 'write-json-file';


@sindresorhus FYI: I've changed it back to import * as syntax for the .sync() example.

Reason: Since this is a Typescript definition, it makes more sense to reference the example in the "Typescript" way.

sindresorhus · 2018-08-22T03:42:37Z

index.d.ts

+ * writeJsonFile.sync('foo.json', {foo: true});
+ * console.log('done');
+ */
+export function sync(filepath: string, data: object | number | string, options?: Options): void;


I think we should make object | number | string a named type called JSONStringifyable? It would more clearly explain why it randomly only accepts object/number/string.

Ok! I'll add it

sindresorhus · 2018-08-22T04:03:17Z

index.d.ts

+	 *
+	 * @default false
+	 */
+	sortKeys?: SortKeys | boolean;


I think boolean should be first as it should be sorted by most common usage, and most users will only ever need to set {sortKeys: true}.

👍 sounds good to me

sindresorhus · 2018-08-27T07:16:58Z

This is perfect now. Thanks for your great work and patience, @DenisCarriere 🙌

DenisCarriere · 2018-08-29T19:21:42Z

@sindresorhus You're welcome! Thanks for building awesome open source tools! 🙌

Add TypeScript definition

d2332fd

Fix tests

b391dd4

sindresorhus requested changes Aug 20, 2018

View reviewed changes

Update based on comments

ffa8823

sindresorhus reviewed Aug 20, 2018

View reviewed changes

Add dot & ignore xo

3e683e8

sindresorhus reviewed Aug 20, 2018

View reviewed changes

remove {returns} & fix indentation

5b9a733

DenisCarriere added 2 commits August 20, 2018 16:53

Update example

2d1dd15

Eslint disable

1c9b6e5

DenisCarriere mentioned this pull request Aug 20, 2018

Add TypeScript definition sindresorhus/load-json-file#15

Merged

Fix indent

5a45486

brandon93s reviewed Aug 21, 2018

View reviewed changes

SamVerschueren suggested changes Aug 21, 2018

View reviewed changes

DenisCarriere added 2 commits August 21, 2018 00:50

Convert tabs to spaces

54aebc1

Future commit will convert package.json to tabs

Add defaults

6a15428

SamVerschueren suggested changes Aug 21, 2018

View reviewed changes

DenisCarriere added 2 commits August 21, 2018 09:33

Add new line to .gitignore

d2c300b

Add @default

8d85e0c

Ref: http://usejsdoc.org/tags-default.html

DenisCarriere commented Aug 21, 2018

View reviewed changes

Make data param "strict"

4df67a1

sindresorhus reviewed Aug 22, 2018

View reviewed changes

Add JSONStringifyable

8062f51

brandon93s approved these changes Aug 24, 2018

View reviewed changes

Update index.d.ts

1f282fb

sindresorhus changed the title ~~Add TypeScript definition~~ Add TypeScript definition and require Node.js 6 Aug 27, 2018

sindresorhus merged commit 8a602ea into sindresorhus:master Aug 27, 2018

DenisCarriere deleted the typescript branch August 29, 2018 19:21

Add TypeScript definition and require Node.js 6 #17

Add TypeScript definition and require Node.js 6 #17

Conversation

DenisCarriere commented Aug 20, 2018

sindresorhus commented Aug 20, 2018

DenisCarriere commented Aug 20, 2018

DenisCarriere commented Aug 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 20, 2018

sindresorhus commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 20, 2018

sindresorhus commented Aug 20, 2018

DenisCarriere commented Aug 20, 2018

brandon93s commented Aug 21, 2018

Choose a reason for hiding this comment

zikaari Aug 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zikaari Aug 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenisCarriere Aug 21, 2018 • edited

Choose a reason for hiding this comment

zikaari commented Aug 21, 2018

DenisCarriere commented Aug 21, 2018

sindresorhus commented Aug 21, 2018

zikaari commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zikaari commented Aug 21, 2018

DenisCarriere commented Aug 21, 2018

Choose a reason for hiding this comment

sindresorhus Aug 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 27, 2018

DenisCarriere commented Aug 29, 2018

DenisCarriere commented Aug 20, 2018 •

edited

zikaari Aug 21, 2018 •

edited

zikaari Aug 21, 2018 •

edited

DenisCarriere Aug 21, 2018 •

edited

sindresorhus Aug 22, 2018 •

edited