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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
node_modules
yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added newline

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 {
/**
* Indentation as a string or number of spaces. Pass in null for no formatting.
*/
indent?: string | number | null;
/**
* Detect indentation automatically if the file exists.
*/
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

/**
* Sort the keys recursively. Optionally pass in a compare function.
*/
sortKeys?: boolean | ((a: string, b: string) => number);
/**
* Passed into JSON.stringify.
*/
replacer?: Replacer | Array<number | string>;
/**
* Mode used when writing the file.
*/
mode?: number;
}

/**
* Stringify and write JSON to a file atomically.
*
* Creates directories for you as needed.
*
* @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
* import * as writeJsonFile from '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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, good point 👍

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 guess if you use Babel this example is correct, I'll change it to:

import writeJsonFile from 'write-json-file';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*
* 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.
*
* Creates directories for you as needed.
*
* @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
* import writeJsonFile from 'write-json-file';
*
* (async () => {
* await writeJsonFile('foo.json', {foo: true});
Copy link
Owner

Choose a reason for hiding this comment

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

Incorrect indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

* console.log('done');
* })();
*/
export default function writeJsonFile(filepath: string, data: any, options?: Options): Promise<void>;
6 changes: 4 additions & 2 deletions 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 @@ -62,11 +62,13 @@ const mainSync = (fp, data, opts) => {
return writeFileAtomic.sync(fp, `${json}\n`, {mode: opts.mode});
};

module.exports = (fp, data, opts) => {
const writeJsonFile = (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 = writeJsonFile;
module.exports.default = writeJsonFile;
module.exports.sync = (fp, data, opts) => {
makeDir.sync(path.dirname(fp), {fs});
init(mainSync, fp, data, opts);
Expand Down
12 changes: 9 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
"url": "sindresorhus.com"
},
"engines": {
"node": ">=4"
"node": ">=6"
},
"scripts": {
"test": "xo && ava"
},
"files": [
"index.js"
"index.js",
"index.d.ts"
],
"keywords": [
"write",
Expand Down Expand Up @@ -44,5 +45,10 @@
"ava": "*",
"tempfile": "^2.0.0",
"xo": "*"
}
},
"xo": {
"ignores": [
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed

"index.d.ts"
]
}
}