node-i18n-iso-countries
node-i18n-iso-countries copied to clipboard
chore: switch to TypeScript
This is a work in progress. References #176
Changelog:
- [x] Switch to TypeScript.
- [x] Move type definitions into
types.ts
file. - [x] Remove old
index.d.ts
file and set compiler flag to autogenerate those from src. - [x] Manage to fix the
localeFilter
type definitions ~(Need some help here!)~ casted in the meantime 🧙 . - [x] Setup Mocha to use ts-node/register.
- [x] Setup ESLint + TS + Prettier.
- [x] Cleanup install (I used yarn so looks like it should be npm instead).
- [x] Converted JSON files into TS files for tree-shaking.
Would like some review from @danilofuchs as well :D
@michaelwittig currently on getAlpha3Code
, getName
, etc. when the lib cannot find the corresponding country, it returns undefined
.
This is currently not expressed with the current index.d.ts
file.
So, this PR also fixes https://github.com/michaelwittig/node-i18n-iso-countries/issues/184 and closes https://github.com/michaelwittig/node-i18n-iso-countries/pull/198
I would suggest returning null
instead of undefined
, but that would introduce a breaking change, so it may not be suitable.
Also, this could conflict with the work on https://github.com/michaelwittig/node-i18n-iso-countries/pull/188
Awesome initiative and great job!
The Typescript build is failing on a few places, please take a look at them.
I resolved the ESLint config and also made the build pass by doing some casting because it is not clear some types for getNames
and getName
methods.
this is super hard to review. Have you changed anything expect converting to TS? @danilofuchs suggests that you also fixed a bug here?
this is super hard to review. Have you changed anything expect converting to TS? @danilofuchs suggests that you also fixed a bug here?
As far as I know, only the linked issues which are about return types being inconsistent with d.ts
(type definitions).
So how is this thing going to be published? So far, all I do is npm publish
. I don't think that that's enough now?
I don't have much time to invest in this project at the moment and I'm not involved in any frontend work at the moment. Therefore, I don't feel very confident in merging this before I know how to publish it.
Yeah, this is a work in progress as I forget to setup the build steps for compilation and enable multiple browser supports as well. After that, the npm publish
can be called without issues but wanted to get feedback from other TypeScript users as well :)
@KingDarBoja I see.
When using Typescript (or any preprocessor like Babel), you have to execute a command to generate output .js
files before running npm publish
.
Usually, that would be npm run build
Rebased the branch to get latest changes and also added the build script with the default tsconfig.json
options and some extra for emitting the d.ts
types and load json modules as well. Would be great if someone install it after running the build
script and play around to spot any issue regarding compatibility.
Almost done with the whole switching as I have installed it locally in some app (using verdaccio) and works almost perfectly, just need to manage the langs
directory to be included in the public API to avoid importing it like:
import { getName, registerLocale } from 'i18n-iso-countries';
import { LocaleEN } from 'i18n-iso-countries/dist/langs/en';
registerLocale(LocaleEN);
And rather be like:
import { getName, registerLocale } from 'i18n-iso-countries';
import { LocaleEN } from 'i18n-iso-countries/langs/en';
registerLocale(LocaleEN);
@KingDarBoja I'm not sure I liked the convertion of locale files from JSON to TS. JSON is easier for beginners to use and many open PRs are made based on these files.
TS can still check JSON files with the resolveJsonModules
tsconfig flag, using a cast localeEN as LocaleData
@KingDarBoja I'm not sure I liked the convertion of locale files from JSON to TS. JSON is easier for beginners to use and many open PRs are made based on these files.
TS can still check JSON files with the
resolveJsonModules
tsconfig flag, using a castlocaleEN as LocaleData
Does this flag let tree-shaking work with json files? My only issue is that I wasn't taking into account the lang files in the build step but since those are json files, I don't think tree-shaking applies for these.
If that's the case, that means converting JSON files to JS or something that can be tree-shakable with tools like Webpack or Rollup but never tried such ways as TS compiler can do the job with TS files.
@danilofuchs after trying out some stuff, I made it work as you expected while keeping the langs
directory files as .json
:
import { getName, registerLocale } from 'i18n-iso-countries';
import { COUNTRY_CODES } from 'i18n-iso-countries/codes';
import { localeEN } from 'i18n-iso-countries/bundled';
Kinda based on the #188 but without rollup, while relying on some workarounds due to TS lack of support for subpath exports from NodeJS v12+.
package.json
{
"typings": "dist/index.d.ts",
"main": "dist/entry-node.js",
"browser": {
"./dist/entry-node.js": "./dist/index.js"
},
"exports": {
".": {
"import": "./dist/index.js"
},
"./codes": {
"import": "./dist/codes.js"
},
"./bundled": {
"import": "./dist/entry-node.js"
}
},
"typesVersions": {
"*": {
"codes": ["dist/codes"],
"bundled": ["dist/entry-node"]
}
},
}
Still have some errors on the consumer side but guess it is a progress. Looks like it is caused by webpack 4 (as I am testing it in a Angular 11 project):
Module not found: Error: Can't resolve 'i18n-iso-countries/bundled'
Module not found: Error: Can't resolve 'i18n-iso-countries/codes'
Update 1
At the end, decided to keep the TS conversion of those json files as it gave me some headaches trying to structure into the final bundle.
Also managed to make the langs imports to work as below in some Angular 11 app I used for playing around:
import { getName, registerLocale } from 'i18n-iso-countries';
import { LocaleEN } from 'i18n-iso-countries/langs';
@Component({ })
export class AppComponent {
constructor() {
registerLocale(LocaleEN);
console.log(getName('co', 'en')); // Colombia
console.log(getName('co', 'es')); // undefined
}
}
Only downside is that I exported all locales in a barrel index.ts
file, so tree-shaking those unused locales doesn't work as it can be seen at below report generated by source-map-explorer
:
Update 2
After tweaking a bit the base tsconfig
to change module
option from commonjs
to ES2015
, now tree-shaking works for those projects using webpack 4 under the hood (Angular as example).
Any more progress / work arounds for using a single locale?, especially with angular where it builds an entire app per locale, having every locale in the bundle is a bit of a bloat
Any more progress / work arounds for using a single locale?, especially with angular where it builds an entire app per locale, having every locale in the bundle is a bit of a bloat
While waiting for this PR, I am making a flag pack library that relies on this package but to enable tree-shaking of unused lang files, I have copied the source code into its own library (not released yet): https://github.com/KingDarBoja/ngx-nations
Probably gonna call it @nation/i18n
as it is not angular specific whereas @nation/ngx-core
will be ofc Angular support 👯♂️
Any news regarding this PR?
any plans to merge this into master at some point?
@bschnabel I don't think that the work here is done. More like a WIP. Correct me if I'm wrong @KingDarBoja ?
@bschnabel I don't think that the work here is done. More like a WIP. Correct me if I'm wrong @KingDarBoja ?
Almost 2 years have passed since last update so I can't remember if there was something missing. Probably not, maybe some conflict updates there and there but I am no longer interested in continuing with this PR (no free time for it).
Hope someone else completes this one if the TypeScript conversion is still wanted 😄
@KingDarBoja I see. Let me close this PR.