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

Add TypeScript definition and require Node.js 6 #17

Merged
merged 15 commits into from
Aug 27, 2018
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
language: node_js
node_js:
- '10'
- '8'
- '6'
- '4'
54 changes: 54 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
type Replacer = (key: string, value: any) => void;

interface Options {
indent?: string | number | null;
detectIndent?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

sortKeys?: boolean | ((a: string, b: string) => number);
replacer?: Replacer | Array<number | string> | null;
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be null.

mode?: number;
}

/**
* Stringify and write JSON to a file atomically
Copy link
Owner

Choose a reason for hiding this comment

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

Should end in a dot.

*
* Creates directories for you as needed.
*
* @param {string} filepath Filepath
* @param {any} data Data
* @param {object} [options] Optional parameters
* @param {string|number|null} [options.indent='\t'] Indentation as a string or number of spaces. Pass in null for no formatting.
* @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.
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

* @returns {void}
Copy link
Owner

Choose a reason for hiding this comment

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

* @example
* const writeJsonFile = require('write-json-file');
Copy link
Owner

Choose a reason for hiding this comment

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

Use import

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

Choose a reason for hiding this comment

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

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.

Copy link

@zikaari zikaari Aug 21, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

@zikaari zikaari Aug 21, 2018

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 for making it more strict in the data param.

There's two options:

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

screen shot 2018-08-21 at 9 57 48 am

  1. 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).

screen shot 2018-08-21 at 9 59 41 am


/**
* Stringify and write JSON to a file atomically
Copy link
Owner

Choose a reason for hiding this comment

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

Should end in a dot.

*
* Creates directories for you as needed.
*
* @param {string} filepath Filepath
* @param {any} data Data
* @param {object} [options] Optional parameters
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* @param {string|number|null} [options.indent='\t'] Indentation as a string or number of spaces. Pass in null for no formatting.
* @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.
* @returns {Promise<void>}
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* @example
* const writeJsonFile = require('write-json-file');
*
* writeJsonFile('foo.json', {foo: true}).then(() => {
* console.log('done');
* });
Copy link
Owner

Choose a reason for hiding this comment

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

Use this instead:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I like this example as well

*/
export default function writeJsonFile(filepath: string, data: any, options?: Options): Promise<void>;
8 changes: 7 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const main = (fp, data, opts) => {
};

const mainSync = (fp, data, opts) => {
let indent = opts.indent;
let {indent} = opts;

if (opts.detectIndent) {
try {
Expand All @@ -67,6 +67,12 @@ module.exports = (fp, data, opts) => {
.then(() => init(main, fp, data, opts));
};

// Support for Typescript default export
module.exports.default = (fp, data, opts) => {
return makeDir(path.dirname(fp), {fs})
.then(() => init(main, fp, data, opts));
};
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 makes sense


module.exports.sync = (fp, data, opts) => {
makeDir.sync(path.dirname(fp), {fs});
init(mainSync, fp, data, opts);
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@
"url": "sindresorhus.com"
},
"engines": {
"node": ">=4"
"node": ">=6"
},
"scripts": {
"test": "xo && ava"
},
"files": [
"index.js"
"index.js",
"index.d.ts"
],
"types": "index.d.ts",
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"keywords": [
"write",
"json",
Expand Down