yashmahalwal commented 4 years ago

Added typings

This PR adds type definitions to the module. It also adds typescript as a development dependency.

Changes that were made

Code was migrated to typescript - under index.ts.
Test script was changed to build typescript code everytime before running tests.
.npmignore added which prevents index.ts from being uploaded to npm.

Caveats
The original module was written with CommonJS module syntax. For backward compatibility, export is currently made as
```
export = Graph
```
So, to use the library in a typescript codebase, you need the esModuleInterop flag enabled. This flag needs to be used with allowSyntheticDefaultImports to supress typecheking import error. That is pretty much a standard way of handling things (moment and express are a few examples that come to the mind)
The codebase could be enhanced by using more of ES6 functionality like Sets and Maps. But that does not affect the overall performance by much and therefore, I do not consider it to be a limitation.
.gitignore does not ignore the built files - index.d.ts and index.js. I leave that choice upto maintainers.
The package needs to be built using npm run build before being published. I have not added that to the prepublishOnly command. In case you feel the need to do so, you can add the following key to package.json
```
{
"scripts": {
...
"prepublishOnly": "npm run build"
}
}
```
Again, I leave that choice up to the maintainers.

curran commented 4 years ago

Thanks for this! Happy that it maintains backward compatibility. Reviewing in detail now...

curran commented 4 years ago

yashmahalwal commented 4 years ago

Looks cool. I will upgrade the library in my project and see how it goes. I did use it extensively so this is a big plus for dev experience :)

curran commented 4 years ago

Excellent! Thank you so much for your inputs here.