jackocnr / intl-tel-input

A JavaScript plugin for entering and validating international telephone numbers
https://intl-tel-input.com
MIT License
7.54k stars 1.94k forks source link

fix: Add missing type definition. Allows to autoconfigure typescript compiler #1561

Closed mpalourdio closed 4 months ago

mpalourdio commented 4 months ago

Hi,

The type entry has been removed from package.json in v21.0.6. This should be re-introduced for consumers. Without it, we need to modify tsconfig.json at userland. Which has already been broken by renaming the file between 21.0.5 and 21.0.6

"paths": {
      "intl-tel-input": [
        "node_modules/intl-tel-input/build/js/intlTelInput"
      ]
    },

This is needed today, but shouldn't (I do not use React, but angular FWIW)

See one of my comment here : https://github.com/mpalourdio/intl-tel-input-ng/pull/54#discussion_r1554837350

jackocnr commented 4 months ago

I removed this because I read that if the types file lives next to the JS file and has the same name, then that should be enough for the code editor to automatically pick it up, and you shouldn't need a types entry in package.json. Additionally, we now have multiple types files, and it seemed wrong to specify only one of them here. With that said, I'm willing to try re-adding this if it solves a problem for you, and we can review if it causes any issues for other people (e.g. who are importing the react component, which has its own types file).

jackocnr commented 4 months ago

Released in v21.0.8

mpalourdio commented 4 months ago

Thanks, just tried 21.0.8 and does the job for me et least. I think you can handle it in other ways of you want to have multiple types definition, like publishing them in the @types dedicated repository, but that's indeed more work.

Also, that's not only about code editor, it's all about typescript compilation. Without them, I have erros like

Error: src/lib/components/intl-tel-input.component.ts:12:46 - error TS7016: Could not find a declaration file for module 'intl-tel-input'. xxxxxx/intl-tel-input-ng/node_modules/intl-tel-input/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/intl-tel-input` if it exists or add a new declaration (.d.ts) file containing `declare module 'intl-tel-input';`

This is fixable at userland side by modifying tsconfig.json, but that's not plug and play, and discouraging for newcomers IMO.

I suppose 21.0.8 will fix https://github.com/jackocnr/intl-tel-input/issues/1558 too

Thanks.

mpalourdio commented 4 months ago

Also, someone is maintaining an outdated @types/intl-tel-input in the official repository, this will lead to extreme pain in the future

jackocnr commented 4 months ago

just tried 21.0.8 and does the job for me et least

👍

I suppose 21.0.8 will fix https://github.com/jackocnr/intl-tel-input/issues/1558 too

Unfortunately not, as that issue is about importing the react component, which has a separate types file

mpalourdio commented 4 months ago

yes, maybe you should export react typings too. Also, this user has grabbed@types/intl-tel-input, which is incompatible

jackocnr commented 4 months ago

maybe you should export react typings too

Well it seems to be working for me without any explicit export configuration, as I said before, just by having the types file in the same dir as the JS file, and with the same name. This is successfully pulling in the types in my own react project.

jackocnr commented 4 months ago

(tho I am quite new to declaration files, so I'm very open to any feedback on the setup, especially with serving 2 separate declaration files with this package)

mpalourdio commented 4 months ago

Maybe you have something very relax in your tsconfig so it finds the definition,

something like

  "include": [
    "**/*.d.ts" <===
  ]

From the doc https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html

TypeScript replicates the node resolution for modules in a package.json, with an additional step for finding .d.ts files. Roughly, the resolution will first check the optional types field, then the "main" field, and finally will try index.d.ts in the root.

Also the publish doc

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

I have tried with 21.0.6, and anther way is setting the main, as stated in the doc. With this, the types is not needed

"main": "build/js/intlTelInput.js", so there's not much magic

mpalourdio commented 4 months ago

@jackocnr Also, regarding your question about multiple .d.ts, here is how RxJs does it for example

{
  "name": "rxjs",
  "version": "7.8.1",
  "description": "Reactive Extensions for modern JavaScript",
  "main": "./dist/cjs/index.js",
  "module": "./dist/esm5/index.js",
  "es2015": "./dist/esm/index.js",
  "types": "index.d.ts",
  "typesVersions": {
    ">=4.2": {
      "*": [
        "dist/types/*"
      ]
    }
  },
  "sideEffects": false,
  "exports": {
    ".": {
      "types": "./dist/types/index.d.ts",
      "node": "./dist/cjs/index.js",
      "require": "./dist/cjs/index.js",
      "es2015": "./dist/esm/index.js",
      "default": "./dist/esm5/index.js"
    },
    "./ajax": {
      "types": "./dist/types/ajax/index.d.ts",
      "node": "./dist/cjs/ajax/index.js",
      "require": "./dist/cjs/ajax/index.js",
      "es2015": "./dist/esm/ajax/index.js",
      "default": "./dist/esm5/ajax/index.js"
    },
    "./fetch": {
      "types": "./dist/types/fetch/index.d.ts",
      "node": "./dist/cjs/fetch/index.js",
      "require": "./dist/cjs/fetch/index.js",
      "es2015": "./dist/esm/fetch/index.js",
      "default": "./dist/esm5/fetch/index.js"
    },
    "./operators": {
      "types": "./dist/types/operators/index.d.ts",
      "node": "./dist/cjs/operators/index.js",
      "require": "./dist/cjs/operators/index.js",
      "es2015": "./dist/esm/operators/index.js",
      "default": "./dist/esm5/operators/index.js"
    },
    "./testing": {
      "types": "./dist/types/testing/index.d.ts",
      "node": "./dist/cjs/testing/index.js",
      "require": "./dist/cjs/testing/index.js",
      "es2015": "./dist/esm/testing/index.js",
      "default": "./dist/esm5/testing/index.js"
    },
    "./webSocket": {
      "types": "./dist/types/webSocket/index.d.ts",
      "node": "./dist/cjs/webSocket/index.js",
      "require": "./dist/cjs/webSocket/index.js",
      "es2015": "./dist/esm/webSocket/index.js",
      "default": "./dist/esm5/webSocket/index.js"
    },
    "./internal/*": {
      "types": "./dist/types/internal/*.d.ts",
      "node": "./dist/cjs/internal/*.js",
      "require": "./dist/cjs/internal/*.js",
      "es2015": "./dist/esm/internal/*.js",
      "default": "./dist/esm5/internal/*.js"
    },
    "./package.json": "./package.json"
  },
  [...]
}

So I guess you could give this a try (kind of, not tested)

"exports": {
    ".": {
      "default": "./build/js/intlTelInput.js",
      "types": "./build/js/intlTelInput.d.ts"
    },
    "./data": "./build/js/data.js",
    "./react": {
      "default": "./react/build/IntlTelInput.js",
      "types": "./react/build/intlTelInput.d.ts"
    },
    "./*": "./*",
  }
jackocnr commented 4 months ago

Thanks for your input. If we have any problems I will definitely try this out 👍