voltace / browser-cookies

Tiny cookies library for the browser
The Unlicense
89 stars 18 forks source link

Add TypeScript definitions #14

Closed nippur72 closed 6 years ago

nippur72 commented 6 years ago

This PR adds type definitions for TypeScript that will be automatically installed by npm.

voltace commented 6 years ago

Thanks for your contribution!

I'd like to add your type definition, though I have a question about how the definitions are exported in the package.json:

    "main": "./lib/main.js",
    "types": "./lib/main.d.ts"

This browser-cookies library is currently a CommonJS module, however I'd also like to export an ES6 version of the library in the future which is possible using the common tag in the package.json:

    "main": "./lib/main.js",
    "common": "./lib/main.module.js"

Do you perhaps know how the typescript definitions can be exported such that both CommonJS and ES6 modules are supported?

nippur72 commented 6 years ago

The definitions are module-agnostic, so it should work out of the box.

The only (minor) thing that might change is how the library is be consumed:

// typescript code
import cookies = require("browser-cookies"); // valid in commonjs but not es6

import * as cookies from "browser-cookies"; // valid in all module systems
voltace commented 6 years ago

If the type definitions are module agnostics then I'm OK with integrating this pull request 👍

I've reviewed your code changes and all the type definitions seem correct. Do you think it would be possible to add the data structure returned by cookies.all() to the type definitions? For example:

interface AllCookiesObject {
  [key: string]: string;
}
export function all(): AllCookiesObject;

Not sure if the example above is correct, you're the typescript expert here.

nippur72 commented 6 years ago

yes, the definition for all() is correct, perhaps in its inlined version:

export function all(): { [key: string]: string };

(which I've already pulled).

voltace commented 6 years ago

Thanks @nippur72 !