Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: switch to TypeScript #218

Conversation

KingDarBoja
Copy link
Contributor

@KingDarBoja KingDarBoja commented Dec 12, 2020

This is a work in progress. References #176

Changelog:

  • Switch to TypeScript.
  • Move type definitions into types.ts file.
  • Remove old index.d.ts file and set compiler flag to autogenerate those from src.
  • Manage to fix the localeFilter type definitions (Need some help here!) casted in the meantime 🧙 .
  • Setup Mocha to use ts-node/register.
  • Setup ESLint + TS + Prettier.
  • Cleanup install (I used yarn so looks like it should be npm instead).
  • Converted JSON files into TS files for tree-shaking.

@KingDarBoja
Copy link
Contributor Author

Would like some review from @danilofuchs as well :D

Copy link
Contributor

@danilofuchs danilofuchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome initiative and great job!

The Typescript build is failing on a few places, please take a look at them.

.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
.eslintrc Show resolved Hide resolved
@danilofuchs
Copy link
Contributor

@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 #184 and closes #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 #188

@KingDarBoja
Copy link
Contributor Author

KingDarBoja commented Dec 12, 2020

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.

@michaelwittig
Copy link
Owner

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

@KingDarBoja
Copy link
Contributor Author

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

@michaelwittig
Copy link
Owner

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.

@KingDarBoja
Copy link
Contributor Author

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

@michaelwittig
Copy link
Owner

@KingDarBoja I see.

@danilofuchs
Copy link
Contributor

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

@KingDarBoja
Copy link
Contributor Author

KingDarBoja commented Dec 24, 2020

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
Copy link
Contributor Author

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

@danilofuchs
Copy link
Contributor

@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
Copy link
Contributor Author

KingDarBoja commented Apr 2, 2021

@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
Copy link
Contributor Author

KingDarBoja commented Apr 2, 2021

@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

@cwqt
Copy link
Contributor

cwqt commented Jun 24, 2021

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

@KingDarBoja
Copy link
Contributor Author

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

@Yberion
Copy link

Yberion commented Dec 6, 2021

Any news regarding this PR?

@bschnabel
Copy link

any plans to merge this into master at some point?

@michaelwittig
Copy link
Owner

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

@KingDarBoja
Copy link
Contributor Author

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

@michaelwittig
Copy link
Owner

@KingDarBoja I see. Let me close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants