Add TypeScript definition #15
Conversation
|
Issue again with $ npm t
> load-json-file@5.0.0 test /Users/denis/Github/load-json-file
> xo && ava
index.d.ts:1:1
✖ 1:1 Parsing error: The keyword interface is reserved
1 error
npm ERR! Test failed. See above for more details. |
|
Updated PR based on comments from => sindresorhus/write-json-file#17 CC: @sindresorhus |
index.d.ts
Outdated
| * console.log('done'); | ||
| * })(); | ||
| */ | ||
| export default function loadJsonFile(filepath: string, options?: Options): Promise<void>; |
There was a problem hiding this comment.
It returns a Promise<any>. What we can do however is make it generic like this
export default function loadJsonFile<T = any>(filepath: string, options?: Options): Promise<T>;Then the user can supply the type that it returns.
interface Foo {
value: string;
}
const data = await loadJsonFile<Foo>('data.json');
//=> data is typed as `Foo`There was a problem hiding this comment.
👍 you're correct, it doesn't return Promise<void> 🤦♂️
I like using the generic approach 👍
index.d.ts
Outdated
| * loadJsonFile.sync('foo.json') | ||
| * console.log(json); | ||
| */ | ||
| export function sync(filepath: string, options?: Options): void; |
There was a problem hiding this comment.
Same as below, it returns an any object but we can make it a generic.
index.d.ts
Outdated
| * @example | ||
| * import * as loadJsonFile from 'load-json-file'; | ||
| * | ||
| * loadJsonFile.sync('foo.json') |
There was a problem hiding this comment.
const json = loadJsonFile.sync('foo.json');
index.d.ts
Outdated
| * | ||
| * (async () => { | ||
| * await loadJsonFile('foo.json'); | ||
| * console.log('done'); |
There was a problem hiding this comment.
Should we use the same example as above and do console.log(json) instead? Because the functionality is the same, it's just async.
There was a problem hiding this comment.
Agreed! I've updated both examples to reflect the same logic
|
@SamVerschueren Pushed updates based on your comments. Thanks for reviewing! |
index.d.ts
Outdated
| /** | ||
| * Applies a function to the JSON string before parsing. | ||
| */ | ||
| beforeParse?: Function |
There was a problem hiding this comment.
I think we can be more specific on what the Function is and should return.
beforeParse?: (data: string) => string;There was a problem hiding this comment.
Totally agree 😄
index.d.ts
Outdated
| * Prescribes how the value originally produced by parsing is transformed, before being returned. | ||
| * See the [JSON.parse docs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Using_the_reviver_parameter) for more. | ||
| */ | ||
| reviver?: Function |
There was a problem hiding this comment.
Same as above, we can be more specific.
reviver?: (key: string, value: any) => any;
|
@SamVerschueren Updated |
index.d.ts
Outdated
| * @example | ||
| * import * as loadJsonFile from 'load-json-file'; | ||
| * | ||
| * const json = loadJsonFile.sync('foo.json') |
index.d.ts
Outdated
| * import * as loadJsonFile from 'load-json-file'; | ||
| * | ||
| * const json = loadJsonFile.sync('foo.json') | ||
| * console.log(json); |
index.d.ts
Outdated
| * //=> {foo: true} | ||
| * })(); | ||
| */ | ||
| export default function loadJsonFile<T = any>(filepath: string, options?: Options): Promise<T>; |
There was a problem hiding this comment.
Should we use unknown here instead of any?
There was a problem hiding this comment.
🤔 unknown isn't a type in Typescript, any would be considered the "unknown" since any can be applied to anything.
There was a problem hiding this comment.
Oh I didn't know this was a new type in Typescript 3.x
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html
Learn something every day!
index.d.ts
Outdated
| * console.log(json); | ||
| * //=> {foo: true} | ||
| */ | ||
| export function sync<T = any>(filepath: string, options?: Options): T; |
There was a problem hiding this comment.
filepath => filePath (I plan to make the same change in the code and docs after merging this)
There was a problem hiding this comment.
Can you add tsd-check? For an example, see write-json-file.
|
@SamVerschueren Sure I'll try to get something started, it won't be as elaborate as yours, but at least it will be a start |
package.json
Outdated
| @@ -36,6 +36,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "ava": "*", | |||
| "tsd-check": "^0.1.0", | |||
There was a problem hiding this comment.
I just released a new version (0.2.1) which supports top-level await.
There was a problem hiding this comment.
👍 awesome! Changed it to "*"
index.d.ts
Outdated
| * //=> {foo: true} | ||
| * })(); | ||
| */ | ||
| export default function loadJsonFile<T = any>(filePath: string, options?: Options): Promise<T>; |
There was a problem hiding this comment.
I think we should use unknown instead of any so users are forced to explicitly coerce it. Thoughts?
There was a problem hiding this comment.
👍 unknown works as the default
Very cool advanced type from Typescript 3.x
|
@sindresorhus @SamVerschueren anything else to review on this PR? |
|
Looks good. Thanks for your work and patience :) |
Add TypeScript definition for
load-json-fileRef: https://github.com/sindresorhus/typescript-definition-style-guide
sindresorhus/ama#439 (comment)