node-i18n-iso-countries icon indicating copy to clipboard operation
node-i18n-iso-countries copied to clipboard

chore: switch to TypeScript

Open KingDarBoja opened this issue 4 years ago • 17 comments

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.

KingDarBoja avatar Dec 12 '20 18:12 KingDarBoja

Would like some review from @danilofuchs as well :D

KingDarBoja avatar Dec 12 '20 19:12 KingDarBoja

@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

danilofuchs avatar Dec 12 '20 20:12 danilofuchs

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.

KingDarBoja avatar Dec 12 '20 20:12 KingDarBoja

this is super hard to review. Have you changed anything expect converting to TS? @danilofuchs suggests that you also fixed a bug here?

michaelwittig avatar Dec 16 '20 09:12 michaelwittig

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).

KingDarBoja avatar Dec 16 '20 12:12 KingDarBoja

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.

michaelwittig avatar Dec 16 '20 13:12 michaelwittig

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 avatar Dec 16 '20 13:12 KingDarBoja

@KingDarBoja I see.

michaelwittig avatar Dec 16 '20 14:12 michaelwittig

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

danilofuchs avatar Dec 16 '20 15:12 danilofuchs

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.

KingDarBoja avatar Dec 24 '20 14:12 KingDarBoja

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 avatar Apr 02 '21 01:04 KingDarBoja

@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

danilofuchs avatar Apr 02 '21 01:04 danilofuchs

@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

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.

KingDarBoja avatar Apr 02 '21 14:04 KingDarBoja

@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:

i18n-angular-first-try

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).

i18n-angular-second-try

KingDarBoja avatar Apr 02 '21 16:04 KingDarBoja

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

cwqt avatar Jun 24 '21 12:06 cwqt

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 👯‍♂️

KingDarBoja avatar Jun 27 '21 19:06 KingDarBoja

Any news regarding this PR?

Yberion avatar Dec 06 '21 14:12 Yberion

any plans to merge this into master at some point?

bschnabel avatar Mar 14 '23 13:03 bschnabel

@bschnabel I don't think that the work here is done. More like a WIP. Correct me if I'm wrong @KingDarBoja ?

michaelwittig avatar Mar 14 '23 13:03 michaelwittig

@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 avatar Mar 14 '23 18:03 KingDarBoja

@KingDarBoja I see. Let me close this PR.

michaelwittig avatar Mar 14 '23 18:03 michaelwittig