#16 Written error class in typescript #19
Conversation
lakhansamani
commented
Sep 21, 2017
lakhansamani
commented
- This branch contains apollo-error code in Typescript
src/index.ts
Outdated
| import ExtendableError from 'es6-error'; | ||
|
|
||
| import * as assert from 'assert'; | ||
| import ExtendableError from 'extendable-error'; | ||
| const isString = d => Object.prototype.toString.call(d) === '[object String]'; |
There was a problem hiding this comment.
Can you skip a line between the imports and the isString definition?
Makefile
Outdated
| @@ -13,7 +13,7 @@ endif | |||
| .FORCE: | |||
|
|
|||
| all: clean | |||
| babel src -d dist --source-maps | |||
| npm run build | |||
There was a problem hiding this comment.
Can we run the command directly from the makefile rather than from npm scripts? The PATH environment variable will already be set from the Makefile, so you can run project-scoped npm binaries from Makefile without having to use npm scripts
There was a problem hiding this comment.
Sure will do that
package.json
Outdated
| "es6-error": "^4.0.0" | ||
| "assert": "^1.4.1", | ||
| "extendable-error": "^0.1.5", | ||
| "install": "^0.10.1" |
There was a problem hiding this comment.
"extendable-error": "^0.1.5"is required for parent class of ApolloError, so it typescript version of es6-error
"assert": "^1.4.1" is required for asserting types in typescript
Have removed install
package.json
Outdated
| @@ -4,7 +4,8 @@ | |||
| "description": "Machine-readable custom errors for Apollostack's GraphQL server", | |||
| "main": "dist/index.js", | |||
| "scripts": { | |||
| "test": "make test" | |||
| "test": "make test", | |||
| "build": "tsc" | |||
There was a problem hiding this comment.
Let's stick to the makefile for the build step. I added the test script to run make test just to keep things canonical with npm init
src/index.ts
Outdated
| super(m); | ||
| name: string; | ||
| message: string; | ||
| time_thrown: any; |
There was a problem hiding this comment.
Can we type time_thrown? That should also be in ISODate (string)
src/index.ts
Outdated
| data: any; | ||
| path: any; | ||
| locations: any; | ||
| _showLocations: any; |
There was a problem hiding this comment.
We should probably type this as boolean and make sure that it is set or defaulted to false
src/index.ts
Outdated
| message: string; | ||
| time_thrown: any; | ||
| data: any, | ||
| options: any, |
There was a problem hiding this comment.
Should we explicitly type the options? That seems like a good idea?
src/index.ts
Outdated
|
|
||
| interface ErrorConfig { | ||
| message: string; | ||
| time_thrown: any; |
There was a problem hiding this comment.
Can we explicitly type time_thrown? See the comment on the class property
src/index.ts
Outdated
| return error; | ||
| } | ||
| } | ||
|
|
||
| export const isInstance = e => e instanceof ApolloError; | ||
|
|
||
| export const createError = (name, config) => { | ||
| export const createError = (name, config: ErrorConfig) => { |
There was a problem hiding this comment.
Can we type name to string?
tsconfig.json
Outdated
| "moduleResolution": "node", | ||
| "sourceMap": true, | ||
| "emitDecoratorMetadata": true, | ||
| "experimentalDecorators": true, |
There was a problem hiding this comment.
Can we prune this down at all? We don't need experimental decorator support, etc.
* Adds build script in make file * Removes un-used modules from package.json * Changes type of time_thrown and _showLocation * Gives default type as string to name parameter in constructor and createError * Removes unsued configurations from tsconfig
|
@thebigredgeek have made the requested changes, please review and let me know if there are any other |
