Refactor: rewrite code in Typescript #13
Conversation
__tests__/babel-wrapper.js
Outdated
| @@ -0,0 +1,3 @@ | |||
| module.exports = require('babel-jest').createTransformer({ | |||
| rootMode: 'upward', | |||
There was a problem hiding this comment.
I don't think you need that file, it's only needed when running jest in a mono-repo (using yarn workspaces)
build/index.js
Outdated
| @@ -0,0 +1,117 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
Hm I don't think built files should be versioned. My understanding with ts libraries is that they publish the built files to npm but only version ts files
countries.ts
Outdated
| @@ -0,0 +1,9532 @@ | |||
| import { CountryRaw } from './types'; | |||
|
|
|||
| const countries: { [countryCode: string]: CountryRaw } = { | |||
There was a problem hiding this comment.
I think you should drop the annotation, as it would prevent typescript to actually infer the valid keys (unless you don't want typescript to actually autocomplete country codes)
| const countries: { [countryCode: string]: CountryRaw } = { | |
| const countries = { |
package.json
Outdated
| "test": "jest --env=node --colors", | ||
| "lint": "eslint --ext ts ./", | ||
| "build": "tsc", | ||
| "typecheck": "tsc" |
There was a problem hiding this comment.
Usually typecheck uses --noEmit so it only checks types without emitting js files. Otherwise it's exactly the same as build
index.ts
Outdated
| * Add search function to each country and map each country by alpha2 | ||
| */ | ||
| const listCountries = keyBy(cloneDeep(countriesRaw), 'alpha2'); | ||
| const countries: { [countryCode: string]: Country } = Object.keys(listCountries).reduce( |
There was a problem hiding this comment.
This means any script importing the library will run this reduce on all countries. Is this an API that you need to support for backwards compatibility? Couldn't it be changed to something like getProvinceByName(countries.US) ?
There was a problem hiding this comment.
it was for backwards compatibility but you're right it's better to remove it
|
I ran |
Refactor
.tsin.jsfor retro compatibility